-
Notifications
You must be signed in to change notification settings - Fork 653
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
atomname methods can handle empty groups #4529
Conversation
Linter Bot Results:Hi @kainszs! Thanks for making this PR. We linted your code and found the following: There are currently no issues detected! 🎉 |
Hi @kainszs , welcome to MDAnalysis! Please ping me whenever you need a review for this PR. |
Thank you for your offer to review the PR @yuxuanzhuang . What are you still missing for the review? The tests and the changelog? I'll add them tomorrow and get back to you. |
I noticed the test (main tests) all failed. It indicates something was wrong with your PR (or at least it broke something that worked before). If you click on Details, you will see which tests fail. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4529 +/- ##
===========================================
- Coverage 93.66% 93.64% -0.02%
===========================================
Files 168 180 +12
Lines 21240 22327 +1087
Branches 3913 3917 +4
===========================================
+ Hits 19894 20908 +1014
- Misses 888 961 +73
Partials 458 458 ☔ View full report in Codecov by Sentry. |
Hello @yuxuanzhuang, I think the PR is ready for a review. I have noticed that some tests also fail in other PRs. In PR #4524 you mentioned that the linter and main_tests (ubuntu-latest, 3.12, true, true) may fail, but what about codecov/project? |
The codecov (coverage) also looks fine. Don't worry about the current failed checks as they also seem to be unrelated to your PR. What you also need to add to the PR is
|
@kainszs The comments regarding what to be add in the changelog somehow are not showing up in this PR, but you need to add an entry in package/CHANGELOG. You need to add to the * 2.8.0 entry block; add a reference to the issue and pr (see the others as examples) and your name in line17. |
So now I am officially an open source developer. ;) @yuxuanzhuang thank you for your time. |
@yuxuanzhuang given that you already commented on the PR, can you please review/shepherd the PR? If you have too much to do, please say something/suggest someone else. 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.
lgtm! please also fix the conflicts.
Congratulations to your first merged PR @kainszs ! 🎉 Thank you for your contribution! |
Congrats! @kainszs |
Fixes #2879
Changes made in this Pull Request:
psi_selections()
,omega_selections()
,chi1_selections()
can handle empty groupsphi_selections
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4529.org.readthedocs.build/en/4529/