-
Notifications
You must be signed in to change notification settings - Fork 36
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
docs: add pydocsstyle linting and switch to mkdocs #396
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #396 +/- ##
=======================================
Coverage 94.72% 94.72%
=======================================
Files 56 56
Lines 3147 3147
=======================================
Hits 2981 2981
Misses 166 166
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
000ec37
to
4f728ef
Compare
4f728ef
to
54730fc
Compare
6b83770
to
8aae6dd
Compare
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.
Nice job! I left a few comments and suggestions throughout. Some of this touched on docstring content updates as there were minor updates applied to these in certain spots.
Additionally, I'm wondering if we need to update the CONTRIBUTING.md
docs on documentation (very meta 🙂 ). Currently these make mention of Sphinx tooling and procedures.
The software [morpheus](https://software.broadinstitute.org/morpheus/) enables profile visualization in the form of interactive heatmaps. | ||
Pycytominer can convert profiles into a `.gct` file for drag-and-drop input into morpheus. | ||
|
||
```python |
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 noticed in the Read the Docs preview that these Python code blocks don't appear to include syntax highlighting. Is it possible to enable this somehow?
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.
Would this become a duplicate of the content found at the root of the project? If so, is there a way we could dynamically reference the root readme without having to create a duplicate here? I can see how differentiating here might be a good idea, but if the content is a clone, it could be difficult to keep up to date over time (even minor skew here could be confusing to readers).
- git fetch --unshallow || true | ||
# Install poetry | ||
# https://python-poetry.org/docs/#installing-manually | ||
- pip install poetry |
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 might be an external issue: reading through this line and recognizing poetry
as a dependency which manages other dependencies, would it make sense to declare the version range acceptable for this and other installations?
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.
Is this file still required in order to use mkdocs
?
# Disable docstring checks in tests | ||
"D", |
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.
Would it make sense to address this eventually? I can understand how it might be a big task to account for this, but feel it may be healthy to document tests at a certain level (otherwise we may set the precedent that we don't need to explain tests for others).
@@ -84,7 +84,7 @@ def test_normalize_standardize_allsamples(): | |||
Testing normalize pycytominer function | |||
method = "standardize" | |||
meta_features = None | |||
samples="all" | |||
samples="all". |
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.
These changes which add the period at the end of method specification felt a little off, and made me wonder about the value-add of these changes. I'd suggest rolling back these changes unless there's a compelling reason to keep them. I also wasn't sure more generally: was the ruff configuration supposed to ignore testing docstrings?
@echo "📚 Building documentation" | ||
@poetry run sphinx-build docs build | ||
@echo "📚 Serving documentation" | ||
@mkdocs serve |
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.
Would it still make sense to run the build or serve through poetry
?
setup-poetry: true | ||
install-deps: true | ||
- name: Build documentation | ||
run: poetry run mkdocs build --strict |
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.
Is there a way to build this check into a pre-GH-Actions check so developers can avoid surprising results from possible failures here? Mostly I wonder here if there's a pre-commit check which could be included that runs a check that things are prepared in advance.
@kenibrewer - checking in on this PR. Do you think you'll be able to resolve conflicts andrevisit @d33bs review? It would be great to include this in the patch release #461 if possible! |
No worries! Thanks for the heads up |
Description
I was working on #382 and couldn't figure out how to resolve some sphinx build errors with opaque error messages. I decided to take this opportunity to switch our docs engine to mkdocs instead.
What is the nature of your change?
Checklist
Please ensure that all boxes are checked before indicating that a pull request is ready for review.
📚 Documentation preview 📚: https://pycytominer--396.org.readthedocs.build/en/396/