-
Notifications
You must be signed in to change notification settings - Fork 646
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
Fix missing fstring on error message #4638
Conversation
Hello @marinegor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-07-24 16:47:44 UTC |
Linter Bot Results:Hi @marinegor! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4638 +/- ##
===========================================
- Coverage 93.61% 93.59% -0.03%
===========================================
Files 171 183 +12
Lines 21250 22320 +1070
Branches 3936 3937 +1
===========================================
+ Hits 19893 20890 +997
- Misses 898 971 +73
Partials 459 459 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking, but it seems like a test is either not working or doesn't exist.
@@ -319,7 +319,7 @@ def __init__( | |||
): | |||
raise ValueError( | |||
( | |||
"Residue {calpha.residue} contains wrong number of hydrogens: " | |||
f"Residue {calpha.residue} contains wrong number of hydrogens: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this covered by a test? I.e. if it is, why did the error message matching not fail? If it isn't, then we should cover it by a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IAlibay fixed, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing thanks!
Fixes #
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4638.org.readthedocs.build/en/4638/