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

chore: Add snapshot path to error #1975

Merged
merged 6 commits into from
Dec 29, 2024
Merged

Conversation

s4moore
Copy link
Contributor

@s4moore s4moore commented Dec 21, 2024

Added the path of the missing directory to the snapshot error message to aid in debugging for users

Close #1941

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.

Hello Samuel,
On behalf of the team, thank you for your contribution and taking the time to improve Back In Time. We appreciate it. Your PR is a good start.

As you might see in the TravisCI output, the test suite offers two failing tests which are related to each other:

  • FAILED test/test_backup.py::TestBackup::test_cant_backup - AttributeError: 'Snapshots' object has no attribute 'snapshotsFullPath'
  • snapshots.py:832:50: E1101: Instance of 'Snapshots' has no 'snapshotsFullPath' member (no-member)

We recommend to run BIT and its test suite on your local system before submitting a PR. In this case you would might have seen these errors first.

I am sure you will find a solution for these error messages. Don't hesitate to ask if you need more assistance.

Regards,
Christian

@buhtz buhtz added this to the 1.6.0 (upcoming release) milestone Dec 22, 2024
@buhtz buhtz added the PR: Modifications requested Maintenance team requested modifications and waiting for their implementation label Dec 22, 2024
@s4moore
Copy link
Contributor Author

s4moore commented Dec 28, 2024

Hello Christian,

Thank you for for your patience. I have updated the code to pull the snapshotsFullPath from the config instance.

I have run the test suite and there are no issues aside from some unrelated warnings. Please let me know if all okay.

Thanks,
Sam

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.

Hello Samuel,
thank you for fixing this.
I just added minor modifications regarding to our translation team. The code might be ugly but separating the translatable stings this way makes the job easier for our translators.

Regards,
Christian

@buhtz buhtz added PR: Merge after creative-break Merge after creative-break (min. 1 week) and removed PR: Modifications requested Maintenance team requested modifications and waiting for their implementation labels Dec 28, 2024
@s4moore
Copy link
Contributor Author

s4moore commented Dec 28, 2024

Okay great, sorry it's my first time working with translators.. So is that all done then? Is there anything else I need to do? (It's also my first time contributing 😬 )

I was thinking to try to solve "Highlight and report non-existent include directories #1910" next unless you have a better suggestion?

@buhtz
Copy link
Member

buhtz commented Dec 28, 2024

Okay great, sorry it's my first time working with translators..

No need to be sorry. Dealing with translators and their strings is not easy and need a lot of experience including trail and error. I would not expect a contributor to know things like this.

So is that all done then? Is there anything else I need to do? (It's also my first time contributing 😬 )

No, everything is fine. You might see the blue check mark and "1 approval". This is my mark that your PR is fine and ready to merge. I also added the label "PR: Merge after creative break". that means the same. We usually wait some days before merging even if the PR seems to be finished. It is often a good idea to sleep over it one night or more no matter how simple a PR looks like.

To find out how we usually tread PRs please read
"What happens after you opened a PullRequest (PR)?".

I was thinking to try to solve "Highlight and report non-existent include directories #1910" next unless you have a better suggestion?

Mhm... Dealing with GUI stuff is not easy because the code base "smells" and is not well organized.

What is your native language? You might want to contribute translations?

Maybe #1105, #1018

@buhtz buhtz changed the title Fix snapshot err msg chore: Add snapshot path to error Dec 28, 2024
@buhtz buhtz merged commit 2889e31 into bit-team:dev Dec 29, 2024
@s4moore
Copy link
Contributor Author

s4moore commented Jan 2, 2025

My native language is English so I guess that will not be too much help but let me know if I'm wrong on that...

I will take a look at #1105

@buhtz
Copy link
Member

buhtz commented Jan 2, 2025

My native language is English so I guess that will not be too much help but let me know if I'm wrong on that...

Ah, OK. Just let me know if you find English that could be improved. Open an issue for this or directly edit the sources and provide the correction with an PR.

I will take a look at #1105

Good choice I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Merge after creative-break Merge after creative-break (min. 1 week)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing debug-info on paths when backup path structure does not exist
2 participants