-
Notifications
You must be signed in to change notification settings - Fork 648
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 error message for XDR readers #4231
Conversation
- error message for not being able to write lockfile did not insert the filename into message: fixed by using proper f string - changed all .format to modern f string in XDR.py - add explicit test for offsets_filename() - add additional pytest.warns() to tests to catch warnings (reduces warnings output and checks that all expected warnings are raised with proper messages)
FYI, this is what the (small) error looks like in practice
Note how |
Linter Bot Results:Hi @orbeckst! 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 |
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.
Couple of small things, otherwise lgtm
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #4231 +/- ##
========================================
Coverage 93.62% 93.62%
========================================
Files 193 193
Lines 25295 25304 +9
Branches 4063 4064 +1
========================================
+ Hits 23683 23692 +9
Misses 1096 1096
Partials 516 516
☔ View full report in Codecov by Sentry. |
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
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.
cheers, lgtm!
|
||
def test_nonexistent_offsets_file(self, traj): | ||
def test_corrupted_offsets_file(self, traj): |
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.
btw, this shadowed the preceeding test; renaming increased our tests
assert len(record) >= len(warning_messages) | ||
found_messages = [str(r.message) for r in record] | ||
# find at least 2 occurences (>= instead of == for robustness) | ||
assert sum([ref in msg for msg in found_messages |
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.
I looked through the other tests in the file and found the following approach that looks a bit cleaner where we stack pytest.warns:
with (pytest.warns(UserWarning, match="Couldn't save offsets") and
pytest.warns(UserWarning, match="Cannot write")):
self._reader(filename)
I'll rewrite to use that idiom.
…sis into fix-xdr-errormessages
- reduces warnings in test_xdr down to 2 (both related to del) - test for empty selection and missing elements warnings - rewriting more old-school parameter interpolation to f strings
Thanks for the quick review @IAlibay . I apologize, I tacked on a few little things that I couldn't let go when I looked at the code again. |
Co-authored-by: Rocco Meli <r.meli@bluemail.ch>
…sis into fix-xdr-errormessages
One of the runners timed out so I restarted it. |
@RMeli or @IAlibay could one of you please volunteer to captain the PR and eventually merge it? I can do it if you prefer but it's better style (in my opinion) to not be the one merging your own PR (unless it's urgent....). I think it's good for the project when there's visibly someone in charge of a PR. |
lgtm! |
Fixes #
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4231.org.readthedocs.build/en/4231/