-
Notifications
You must be signed in to change notification settings - Fork 19
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
MWTree and MWNode documentation #219
Conversation
docs to classes.rst
If anyone with access can make me admin for the documentation project on readthedocs.org, I can activate the documentation build on pull request. It will be easier to review changes such as these |
I don't seem to be admin for mrcpp on readthedocs. Maybe @stigrj? |
I thought i enabled that already, but it is weird that it did not trigger on this PR. Is it not enough to just enable it in the |
According to the docs, yes, that's enough. Investigating... |
The build starts, but then fails... |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #219 +/- ##
=======================================
Coverage 66.48% 66.48%
=======================================
Files 169 169
Lines 9810 9811 +1
=======================================
+ Hits 6522 6523 +1
Misses 3288 3288
☔ View full report in Codecov by Sentry. |
Seems it needed a re-sync of the github hook. Now it shows the documentation build in the list of PR checks. It's also building the docs. |
Seems you need to debug the build on readthedocs before we can merge this. https://readthedocs.org/projects/mrcpp/builds/22034460/ |
I don't quite get it. The last time I commited the build on readthedocs (which i triggered manually by pushing to the rtd-build branch) was working just fine. So something must have happened in the meantime... |
How did you do this?
did you do a rebase before creating the PR? I think it might be some other issues that are crashing though |
Not clear to me what happened but the doc build seems fine now after I force-pushed my own version |
This PR is btw from the rtd-build which is on upstream (not my own fork, my bad!). |
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 dont see any issues in the documentation build and the tests have passed. This can be merged in my opinion.
By reading the docs 😄 https://docs.readthedocs.io/en/stable/guides/pull-requests.html#troubleshooting |
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 but I leave here some food for thought. In general, I prefer the Doxygen documentation to be only in the header files, because they are readable once the library is installed and I can have a look at the documentation from the source directly (and I think IDEs can read it as well)
It's up to you to decide whether it goes in the header or the source file. But at least do it consistently, not a little bit here and little bit there.
MWTree
andMWNode
classes are now sufficiently documented.Workflow to approve this pull request (and all other PRs about documentation)
NOTE: This PR includes quite a few more changes other than just in the
MWNode
andMWTree
classes because we had to refresh a few config files.