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

Modernize documentation #49

Merged
merged 1 commit into from
May 16, 2024
Merged

Modernize documentation #49

merged 1 commit into from
May 16, 2024

Conversation

AntoineBrunet
Copy link
Contributor

This PR propose a modern rewrite of the full documentation using Sphinx.

This new version can be previewed at https://antoinebrunet.github.io/IRBEM/ . I've tried to keep all the information available in the current documentation, if some parts are missing please mention it. Some parts where slightly re-arranged or re-ordered when I felt it made sense. I believe this updated version of the documentation makes it much easier to find all the relevent informations on the API, and it will be much easier to keep it up to date in the future.

I tried to chose a neutral, modern template for the documentation. If someone has another idea or wants to suggest a different look, I really don't mind.

@mshumko
Copy link
Contributor

mshumko commented May 16, 2024

Thanks @AntoineBrunet for rewriting the documentation! The docs look well-organized and clear (although I am not a good representation of a typical IRBEM user).

I have two general comments

  1. The documentation deployment did not succeed due to insufficient token permissions. I presume that we'll have to redo the authentication steps after this PR is merged.
  2. While the recommendation to use help(python_function) is a good step forward, the docstrings in IRBEM.py can be automatically parsed by sphinx. It is perhaps a task for the next PR that I can work on, but I am wondering on what are the pros/cons to separately documenting the APIs for each language that we support?

@AntoineBrunet
Copy link
Contributor Author

  1. The documentation deployment did not succeed due to insufficient token permissions. I presume that we'll have to redo the authentication steps after this PR is merged.

I think it failed because it ran during the PR. It should work fine after a merge in main (I guess). We should prevent the deploy phase of the job for PRs though, so that this error doesn't show up. I'll fix it asap in this PR.

  1. While the recommendation to use help(python_function) is a good step forward, the docstrings in IRBEM.py can be automatically parsed by sphinx. It is perhaps a task for the next PR that I can work on, but I am wondering on what are the pros/cons to separately documenting the APIs for each language that we support?

I'm not sure about this. Most of the time the routines in all languages have very similar names and parameters, so grouping the documentation by routine and showing calling sequence for each language makes sense to me, and maintaining a separate documentation for each language sounds like extra work and more error prone. That said, specifically for python, you are right that sphinx can automatically generate the documentation, so it could be straightforward to implement (assuming the docstrings are properly written). I think what would be most useful though would be to have user manuals for each languages with short working examples on how to do specific tasks with the library.

Copy link
Contributor

@mshumko mshumko left a comment

Choose a reason for hiding this comment

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

I agree. This PR is already a huge effort so let's leave the python examples for a future PR.

@mshumko mshumko merged commit 921b949 into PRBEM:main May 16, 2024
10 of 11 checks passed
@mshumko
Copy link
Contributor

mshumko commented May 16, 2024

Merged. I see that the https://prbem.github.io/IRBEM/ link already points to the new docs so I am unsure if the token permissions issue is still relevant.

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.

2 participants