-
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
Improve error message when calling AtmGroup.unwrap() without bonds #4642
Improve error message when calling AtmGroup.unwrap() without bonds #4642
Conversation
Hello @laksh-krishna-sharma! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-07-28 09:21:18 UTC |
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.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS
as part of this PR.
Linter Bot Results:Hi @laksh-krishna-sharma! 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.
@laksh-krishna-sharma looks good.
Could you please follow @tylerjereddy 's suggestion and find the place where we test for the exception being raised and then add a match for part of the message.
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.
The tests are failing now, please fix. Perhaps you need to treat the match string more carefully. It would also be ok to just match the first line.
ya, just using a regex-style match on a sub-section of the message is usually what I see done |
Thank you for your feedback, @orbeckst and @tylerjereddy. I have resolved the issue by carefully revising the match string. Please review the updated. |
Tests are failing with something that looks related to your code
Can you please check? |
Thank you for letting me know, @orbeckst. I will investigate the issue and address the failing tests as soon as possible. I'll update once the problem is resolved. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4642 +/- ##
===========================================
- 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. |
Thank you for your feedback, @orbeckst and @tylerjereddy. I have resolved the issue carefully . Please review the updated. |
Please review the updated. |
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.
This looks functionality very good. I have one request for you to improve our existing code (see comment) and also
- add yourself to the end of the file AUTHORS
- update CHANGELOG: add an entry for the change in the CHANGELOG file to the top of Enhancements
Lines 54 to 55 in 7302719
Enhancements * Add `analysis.DSSP` module for protein secondary structure assignment, based on [pydssp](https://github.com/ShintaroMinami/PyDSSP) Line 20 in 7302719
yuxuanzhuang, PythonFZ
That's all I see and then I can merge it.
Thank you for your review, @orbeckst. I'll complete all the tasks mentioned. |
…unwrap(), rewrite test case
Thank you for your feedback, @orbeckst. I have made all the requested improvements, including updating the AUTHORS file, adding an entry to the CHANGELOG under Enhancements, and ensuring my GitHub handle is included with the contributors. I also revised the test using pytest.raises with the match parameter for message checking. Please review the updated. |
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.
Thanks, please see inline comments!
"2. Create a universe using a topology format where bonds are pre-defined." | ||
) | ||
expected_message_pattern = re.escape(expected_message) | ||
assert re.fullmatch(expected_message_pattern, str(exc.value)) |
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.
Sorry, my previous comment wasn't clear enough. What I would like you to do is to
- add to the
pytest.raises(NoDataError)
above amatch=expected_message_pattern
) - remove the
assert
that checks for the message.
In this way we have all checks inside the pytest.raises
.
@orbeckst thank you for clearly mentioning the issue again. I have resolved it carefully. Please review the updated. |
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.
All looking good, thanking you for patiently and promptly and addressing my comments.
I'll merge — thank you for your first contribution! I hope we see more from you!
Congratulations, @laksh-krishna-sharma , first merged contribution! 🎉 |
Thank you, @orbeckst! I am grateful for the opportunity to contribute and for your patient guidance. I would like to work more under your guidance in this repository. It would be great if you could help me with the environment setup, as I am looking to contribute more and learn from you. This being my first contribution, it has been an amazing experience, and I have learned a lot. I am excited to continue contributing and learning. |
Thank you for your contribution, it was a pleasure working with you. We only assign specific mentors for something like Google Summer of Code. When you're part of the MDAnalysis community you'll interact with many people — users, developers, documentation writers, instructors, ... I suggest you engage through the public forums (discussions, discord) if you have specific questions — typically there's someone there who can point you in the right direction. |
Thank you @orbeckst . I understand the mentorship structure within the MDAnalysis community. I look forward to interacting with the community and continuing to contribute to the project. |
Fixes #4436: Improve error message when calling AtomGroup.unwrap() without bonds
This PR improves the error message that is raised when calling AtomGroup.unwrap() without defined bonds. The new message provides clearer guidance on how users can resolve the issue.
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4642.org.readthedocs.build/en/4642/