Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Check if Include folders/files do exists (in case they are removed) #1718

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from

Conversation

rafaelhdr
Copy link
Contributor

Solution b) when start taking a snapshot the include list should be checked of existence first and warn about missings.

reference: #1586

Solution b) when start taking a snapshot the include list should be checked of existence first and warn about missings.

reference: bit-team#1586
@rafaelhdr
Copy link
Contributor Author

rafaelhdr commented May 9, 2024

This is not complete yet, but if possible I would like some feedback before finishing it.

Are the messages good? And I am wondering if the qt warning is ok (it is asking a confirmation before continuing instead of just warning).

And I didn't implement a) for no big reason. I am still getting familiar with the code and b) was something that was more clear for me. I could try a) in a future PR.

And how you normally do translations? I was planning to translate all other languages using some automated tool (Copilot) but I wanted to confirm how you guys do it normally...

CHANGES Outdated Show resolved Hide resolved
common/backintime.py Outdated Show resolved Hide resolved
common/backintime.py Outdated Show resolved Hide resolved
common/backintime.py Outdated Show resolved Hide resolved
common/po/pt_BR.po Outdated Show resolved Hide resolved
common/snapshots.py Outdated Show resolved Hide resolved
qt/app.py Show resolved Hide resolved
qt/app.py Outdated Show resolved Hide resolved
qt/app.py Outdated Show resolved Hide resolved
@buhtz
Copy link
Member

buhtz commented May 9, 2024

Dear Rafael,
Awesome! Thank you for this next contribution.

I added some comments and suggestions to the code. I am not sure about the whole solution. I would suggest to further discuss it in the related issue #1586.

I will set the PR into Draft mode to state the the solution is not finished.

Best,
Christian

rafaelhdr and others added 4 commits May 9, 2024 22:19
Co-authored-by: buhtz <[email protected]>
Co-authored-by: buhtz <[email protected]>
Co-authored-by: buhtz <[email protected]>
Co-authored-by: buhtz <[email protected]>
@rafaelhdr
Copy link
Contributor Author

I appreciate a lot for the review. It clarified a lot of questions and taught me some good lessons (I would never have thought about RTL issues on translations). And I am happy about not use the camel case :)

I will do the fixes from the review and wait for any suggestion in the original issue.

Thank you very much 🙏

@buhtz
Copy link
Member

buhtz commented May 10, 2024

Does anyone know how to trigger the systray-icon message-bubbles via BIT?

I tried with "Snapshot.setTakeSnapshotMessage(1, 'FOO BAR')" but without success. This message appears in the status bar of the main window.

@buhtz
Copy link
Member

buhtz commented May 12, 2024

I would never have thought about RTL issues on translations

Me neither. When I started to work on the translation task I thougt it is easy. I learned things like that from translators and the community around them while trying to attract translators to BIT.

@buhtz buhtz added Discussion decision or consensus needed PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. labels May 13, 2024
@buhtz buhtz changed the title Check if Include folders/files do exists (in case they are removed) feat: Check if Include folders/files do exists (in case they are removed) Nov 19, 2024
@buhtz
Copy link
Member

buhtz commented Nov 23, 2024

Hello,
Just for your information. I do plan the next release (1.6.0) before Christmas.

How is your schedule? Can I plan your PR for this release (1.6.0) or for the release after it (1.6.1)?

Regards,
Christian

@rafaelhdr
Copy link
Contributor Author

Hello

I think it would be great :)

But I am very new here (this is my second Pull Request in the project). It would be great to have someone with more experience and I am available to do any fix.

Copy link
Member

@buhtz buhtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I checked the old review comments.
Now you should see only four pending/open review comments added by me.
You can use the "Commit suggestion" button if you want.

About aryoda's review: I would say all comments from him are solved. But I am not able to "accept" the the changes to his review on the GitHub interface. I can only click "dismiss review", which I don't want to. ;) So just treat his comments as solved.

I also updated your branch to the latest dev version. So make sure you also update your local branch at your working machine.

I would like to do some more manual tests before merging.

Let me know if you have questions or if I can be of assistance.

common/snapshots.py Outdated Show resolved Hide resolved
qt/app.py Outdated Show resolved Hide resolved
common/snapshots.py Outdated Show resolved Hide resolved
common/snapshots.py Outdated Show resolved Hide resolved
@buhtz buhtz removed Discussion decision or consensus needed Feedback needs user response, may be closed after timeout without a response labels Nov 30, 2024
@buhtz
Copy link
Member

buhtz commented Dec 29, 2024

Hello Rafael,
can you give me a ping please. 🔔 What are your plans with this PR?

I suggest you to update your local branch to the latest dev version and integrate that into your PR.

  1. Go to your own "rafaelhdr:backintime" fork at GitHub. Click on "Sync fork".
  2. Go to your local machine and checkout the "dev" branch.
  3. Run git fetch --prune --all.
  4. Run git pull.
  5. Checkout your branch check-included-do-exists.
  6. Run git pull.
  7. Merge dev into your branch via git merge dev.
  8. Resolve merge conflicts if they are there. Don't worry, we can fix them later if somethings goes wrong.
  9. Push it to this PR after committing it via git push.

As an alternative, I am also able to do the merging and conflict resolving on my side. Let me know.

@rafaelhdr
Copy link
Contributor Author

rafaelhdr commented Dec 29, 2024 via email

@buhtz
Copy link
Member

buhtz commented Dec 29, 2024

Great to hear. Don't feel rushed. I just want to know if you are still alive. Take your time.
Have a nice vacation.

@rafaelhdr rafaelhdr marked this pull request as ready for review January 7, 2025 08:03
@rafaelhdr
Copy link
Contributor Author

Thank you. I just made latest fixes from review. I think it is ready again for review.

And I appreciate an extra manual review on it 🙏

@buhtz buhtz added the PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. label Jan 7, 2025
Comment on lines +1293 to +1294
msg = _('The following directories are missing: {dirs} Do you want to proceed?'.format(
dirs=f'\n{msg_missing}\n\n'))
Copy link
Member

@buhtz buhtz Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format() need to be outside of the translatable string.

# Problematic code
_('foo {val}'.format(val='bar'))

# Good code
_('foo {val}').format(val='bar')

Don't worry, this translation stuff is complex.

Suggested change
msg = _('The following directories are missing: {dirs} Do you want to proceed?'.format(
dirs=f'\n{msg_missing}\n\n'))
msg = _('The following directories are missing: {dirs} Do you want to proceed?').format(
dirs=f'\n{msg_missing}\n\n')

Copy link
Member

@buhtz buhtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work. I commented just one minor suggestion.

After that I will test it on my machine.

@@ -41,6 +41,7 @@ Version 1.5.2 (2024-08-06)
* Build(translation): Language helper script processing syntax checks on po-files

Version 1.5.1 (2024-07-27)
* Fix bug: Check if Include folders/files do exists (in case they are removed) (#1586) (@rafaelhdr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can move this entry up to the first section in the changelog.

@buhtz buhtz added HELP-WANTED Used by 24pullrequests.com to suggest issues and removed PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. labels Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HELP-WANTED Used by 24pullrequests.com to suggest issues PR: Modifications requested Maintenance team requested modifications and waiting for their implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants