-
Notifications
You must be signed in to change notification settings - Fork 661
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: groups.py doctest fixes #4374
Conversation
Hello @ljwoods2! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-03-25 17:59:45 UTC |
Linter Bot Results:Hi @ljwoods2! 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.
@ljwoods2 - could we ask you to revert the black
auto formatting please? This not only makes reviews difficult but it also obscures the great work you're doing towards fixing this issue.
Auto formatting the library is an ongoing discussion, if it happens, it will be handled in a future PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4374 +/- ##
===========================================
- Coverage 93.65% 93.63% -0.03%
===========================================
Files 168 180 +12
Lines 21215 22294 +1079
Branches 3908 3908
===========================================
+ Hits 19869 20875 +1006
- Misses 888 961 +73
Partials 458 458 ☔ View full report in Codecov by Sentry. |
This reverts commit 1cd741b.
@IAlibay Done, I reverted a few things line-wise so I may have undone some PEP8 changes as well. In the future, should I change all the files I touch to be PEP-8 compliant, but not Black? Or do I only make the new code that I contribute PEP-8? |
The second one. New code should be PEP-8 compliant, but you should not fix other issues in the same file (will make review murkier). I know it's not great if you have linters on on your editor/IDE because of the large amount of errors you see, but until we decide to reformat the whole code base, this remains the best way forward for the project. If you feel strongly about this, please add your view/experience in #2450. |
Cycling the PR to restart Codecov action. |
Running full deps py3.12 check in github actions now.
Put upper pin at 3.13 not 3.12
Pre-release commit for 2.7.0
Start 2.8.0 changelog
* Fix deploy action, using the correct version number for the pypi upload method.
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 all looks nice to me and seems to work for me locally (make doctest
).
However, I haven't been super-involved with doctesting so I wanted to know from @hmacdope @richardjgowers @IAlibay (people involved in #3925 — other opinions welcome, too, of course!) if using sphinx directives is ok? In other words, are we ok with the following not working:
pytest -v --disable-pytest-warnings --doctest-modules
and only
make doctest
aka
sphinx-build -v -W -b doctest --keep-going source ../doctest
being used for doc-testing?
I'll block the PR until that's clear.
FYI, this is what the passing doctests look like
|
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.
Based on the comments on using sphinx in doc tests from discord, using sphinx is ok so I am approving.
Fixes #3925 in part
Changes made in this Pull Request:
Note: These tests will not pass using "pytest -v --disable-pytest-warnings --doctest-modules " since I used sphinx directives to allow for concise example code. This can be changed but the documentation will become more verbose.
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4374.org.readthedocs.build/en/4374/