-
Notifications
You must be signed in to change notification settings - Fork 709
clean API pages batch 1to4 #4999
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
base: develop
Are you sure you want to change the base?
clean API pages batch 1to4 #4999
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4999 +/- ##
===========================================
- Coverage 93.42% 93.41% -0.02%
===========================================
Files 177 189 +12
Lines 21859 22931 +1072
Branches 3078 3079 +1
===========================================
+ Hits 20422 21421 +999
- Misses 986 1059 +73
Partials 451 451 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I probably won't get to reviewing for a while because there's too much GSOC to do, sorry. Please find another reviewer. |
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!
Only a few very minor comments and nitpicks. (I'm not a native speaker, so please disregard as you see fit.)
package/doc/sphinx/source/documentation_pages/analysis_modules.rst
Outdated
Show resolved
Hide resolved
package/doc/sphinx/source/documentation_pages/analysis_modules.rst
Outdated
Show resolved
Hide resolved
package/doc/sphinx/source/documentation_pages/analysis_modules.rst
Outdated
Show resolved
Hide resolved
Thanks @RMeli for the comments. I made the changes. |
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
@micaela-matta can you please shepherd the PR to completion? |
@namiroues when someone approves your PR, you don't need to ask for their review again. They trusted you you'd do the right thing and they might not have time to just say "yes" again — people often don't have 5 minutes to open a PR and look at it for a LGTM. EDIT: I'll now leave the actual finalizing/merging to @micaela-matta . EDIT: I was wrong, the approval is recorded and not deleted... it's just confusing when you look at the reviewer list and nobody has green tickmark. |
Sorry, I missed this re-review request! I agree with @orbeckst that it is not necessary to re-request a review on approval (unless you completely change the PR). |
I don't think you were wrong. The approval is indeed recorded in the PR history, but it doesn't show up in the top level (PR reviewers, PR list, etc.) and I don't think it counts as a required review to allow merge. |
Fixes #4998
The edits in this PR only apply to non-autogenerated API documentation pages. I did not modify any auto-generated API reference files (e.g., those populated via automodule, autoclass, autofunction, etc.).
All changes were made to manually written .rst files that provide curated narrative content or structured overviews (e.g., analysis.rst, topology.rst, parallelization.rst). These are not overwritten by Sphinx during builds and are safe to edit directly.
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--4999.org.readthedocs.build/en/4999/