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

Doxygen rtd integration [second attempt] #574

Merged
merged 3 commits into from
Mar 17, 2021

Conversation

elbashandy
Copy link
Contributor

I integrated the abstract changes that I made from PR #505 here. The rest of the changes will be added as part of the clean-up process as I have to go through each class and conform it to the Doxygen commentary style. For example, I will make sure that each class has a brief description that Doxygen can pick up. For now, we can add the changes made here in this PR.

@burlen I made the changes that you requested yesterday to the macro expansion comments. Please let me know if something else needs to be done.

@elbashandy elbashandy requested review from burlen and taobrienlbl and removed request for burlen and taobrienlbl March 12, 2021 00:27
@taobrienlbl
Copy link
Collaborator

@elbashandy or @burlen - is it possible to configure RTD to build this branch? I know it looked good on @elbashandy's github.io site, and it would be good to verify that this also looks good on our RTD site before merging.

Pending that, I have no further comments on this PR. As we discussed last week, we'll continue to clean up code in future PRs.

@elbashandy elbashandy force-pushed the doxygen_rtd_second_attempt branch from 01c24e2 to dbd7ec8 Compare March 12, 2021 16:26
@elbashandy
Copy link
Contributor Author

@taobrienlbl on it!

@burlen
Copy link
Collaborator

burlen commented Mar 12, 2021

@elbashandy or @burlen - is it possible to configure RTD to build this branch?

https://teca.readthedocs.io/en/doxygen_rtd_second_attempt/

@taobrienlbl
Copy link
Collaborator

@elbashandy , there are some substantial differences between the amount of information in your github.io version and the RTD version that @burlen linked to. See below:

The RTD version:
Screen Shot 2021-03-12 at 1 54 26 PM

The github.io version:
Screen Shot 2021-03-12 at 1 54 32 PM

Is this expected?

@burlen
Copy link
Collaborator

burlen commented Mar 12, 2021

To recap and clarify: My comments about merging this work sooner rather than later are 2 fold. 1 it's useful to have live doxygen as we make incrementally changes, we can see the results right away. This may shape the doxygen conventions we adopt as we learn what works well and what does not. 2: because your changes had some revisions to class documentation that run the risk of conflicting with my revisions of the class docs that are part of my day to day work.

I think that working on one big patch that updates all classes is going to be problematic because I imagine this will take you quite some time to do and if it's pending for a long time there will inevitably be conflicts between our work. resolving such conflicts will be more work and probably one set of edits will be discarded which is wasteful.

I suggest an incremental approach, merge what you have outstanding in #505 now, as per my review. As you revise make new pr's periodically as needed so that these changes aren't sitting around for too long. the longer they sit the more likely there will be conflicts and the problems mentioned.

so what happened to the tables of algorithms? That was closer to the end goal than the tree view thing. While it's pretty, I'm not sure we need the tree view code in the repo long term which is why I'm hesitant to commit it at all.

@burlen
Copy link
Collaborator

burlen commented Mar 12, 2021

fyi -- the mac build failures are related to changes in our Travis-CI account made by their engineers this morning, not in our codes.

@elbashandy
Copy link
Contributor Author

@taobrienlbl @burlen Now that I can see the teca.readthedocs.io API page, I unselected you both from reviewing as I need to debug why it's not replicating the behavior that I previously demonstrated at elbashandy.github.io (the API pages + tables + hierarchies). The good news is that Doxygen is running on RTD as expected.

@taobrienlbl
Copy link
Collaborator

cool, I'll be glad to review again once you're ready. If you didn't notice, Burlen merged a PR earlier today, so you'll probably need to merge with develop again if you haven't already.

@elbashandy elbashandy force-pushed the doxygen_rtd_second_attempt branch from dbd7ec8 to 2338f87 Compare March 16, 2021 01:04
@elbashandy
Copy link
Contributor Author

The documentation is ready and mimics the same presentation of elbashandy.github.io

https://teca.readthedocs.io/en/doxygen_rtd_second_attempt/api/api.html

Copy link
Collaborator

@burlen burlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, if you rebase develop the builds should pass now

Created an API page

Added in _static a collapsible-list library for tree generation of hierarchies

Included a script that parses Doxygen XML files to generate pages for:
1. Alg, Core, Data, I/O
2. Class Hierarchy
3. File Hierarchy
@elbashandy elbashandy force-pushed the doxygen_rtd_second_attempt branch from 2338f87 to 04f75a4 Compare March 17, 2021 00:11
@elbashandy elbashandy merged commit 842f551 into develop Mar 17, 2021
@burlen burlen deleted the doxygen_rtd_second_attempt branch March 17, 2021 04:48
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.

None yet

3 participants