Skip to content
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

Merged
merged 32 commits into from
Sep 26, 2023
Merged

MWTree and MWNode documentation #219

merged 32 commits into from
Sep 26, 2023

Conversation

ilfreddy
Copy link
Contributor

@ilfreddy ilfreddy commented Sep 25, 2023

MWTree and MWNode classes are now sufficiently documented.

Workflow to approve this pull request (and all other PRs about documentation)

  1. pull the current branch
  2. push it to the rtd-build in master
  3. check that the commit does not alter anything else other than the documentation
  4. check that the documentation produced is sane, meaningful and well written (language, grammar, typos)
  5. check that all tests pass as requested
  6. approve :-)

NOTE: This PR includes quite a few more changes other than just in the MWNode and MWTree classes because we had to refresh a few config files.

@robertodr
Copy link
Contributor

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

@ilfreddy
Copy link
Contributor Author

I don't seem to be admin for mrcpp on readthedocs. Maybe @stigrj?

@Gabrielgerez
Copy link
Contributor

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 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 advanced settings > Build pull requests for this project ?

@robertodr
Copy link
Contributor

According to the docs, yes, that's enough. Investigating...

@robertodr
Copy link
Contributor

The build starts, but then fails...

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (5320eb4) 66.48% compared to head (e5262aa) 66.48%.

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           
Files Coverage Δ
src/operators/IdentityConvolution.cpp 100.00% <ø> (ø)
src/operators/MWOperator.h 75.00% <ø> (ø)
src/trees/MWNode.cpp 80.61% <ø> (-0.04%) ⬇️
src/trees/MWNode.h 71.15% <ø> (ø)
src/trees/MWTree.cpp 79.25% <100.00%> (ø)
src/trees/MWTree.h 72.41% <ø> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robertodr
Copy link
Contributor

robertodr commented Sep 26, 2023

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.

@robertodr
Copy link
Contributor

robertodr commented Sep 26, 2023

Seems you need to debug the build on readthedocs before we can merge this. https://readthedocs.org/projects/mrcpp/builds/22034460/

@ilfreddy
Copy link
Contributor Author

ilfreddy commented Sep 26, 2023

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...

@Gabrielgerez
Copy link
Contributor

Gabrielgerez commented Sep 26, 2023

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.

How did you do this?

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...

did you do a rebase before creating the PR? I think it might be some other issues that are crashing though

@ilfreddy
Copy link
Contributor Author

Not clear to me what happened but the doc build seems fine now after I force-pushed my own version

@ilfreddy
Copy link
Contributor Author

This PR is btw from the rtd-build which is on upstream (not my own fork, my bad!).

Copy link
Contributor

@Gabrielgerez Gabrielgerez left a 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.

@robertodr
Copy link
Contributor

How did you do this?

By reading the docs 😄 https://docs.readthedocs.io/en/stable/guides/pull-requests.html#troubleshooting

Copy link
Contributor

@robertodr robertodr left a 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.

@robertodr robertodr merged commit f9b2a1e into master Sep 26, 2023
6 checks passed
@robertodr robertodr deleted the rtd-build branch September 26, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants