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

Added readthedocs config for documentation #339

Merged
merged 37 commits into from
Jan 8, 2024

Conversation

rakesh9177
Copy link
Contributor

Readthedocs is configures, whenever a commit happens, it will update the docs automatically, make sure to update the readme.md file if new instructions are needed.

Please create an account on readthedocs and add your repo to publish the docs

@rakesh9177
Copy link
Contributor Author

Hi @SagiPolaczek , Please let me know if anything is required on this. Thank you!

@SagiPolaczek
Copy link
Collaborator

@rakesh9177 Awesome, thanks! And sorry for the delay, had to be afk for couple of days.
Will add a review comment now :)

Copy link
Collaborator

@SagiPolaczek SagiPolaczek left a comment

Choose a reason for hiding this comment

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

Thanks again!

Two comments:

  1. The mypy workflow fails. You can (hopefully) see it here. Minor changes in docs/source/conf.py are required.
  2. While the landing page looks good, the others are empty. For example fuse package. This in contrast to other libraries, for example causallib where their landing page looks good but also other submodule have the same format (example).

Do you know what's the reason? Anything to change in our formatting? Or we need to play with the settings of readthedocs (I compared them and it looks the same to me).

@rakesh9177
Copy link
Contributor Author

Hi @SagiPolaczek ,

Thanks for the review, I updated the conf.py file as per mypy.

The landing page for modules requires a Readme.md file within each module package. we can do this by including the path of readme.md file in rst files.
I see there is one for fuseimg but there isn't one for fuse package. Although there is a Readme.md inside fuse.dl, do you want to me to use that for fuse package or I just use fuse.dl as a package and use readme.md file for documentation for this?

Thanks again!

Copy link
Collaborator

@SagiPolaczek SagiPolaczek left a comment

Choose a reason for hiding this comment

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

@rakesh9177 Thanks for addressing it! Great job! :)

Few comments:

  1. As for your question, it can be under fuse.dl (to match the README.md file location and the script you added in conf.py).
  2. I pointed for some changes in the conf.py. I see that the script adds a pointer to the README.md file for each .rst one. Currently it looks like we walk on the wrong path.
  3. There are more format & typing issues. My suggestion is to use pre-commit hooks - it will automatically do the format changes it can, and will enforce you to match the mypy typing format.
    In order to do so just run pre-commit install (first pip install the package of course).

docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/source/fuseimg.rst Show resolved Hide resolved
docs/source/fuse.rst Show resolved Hide resolved
@rakesh9177
Copy link
Contributor Author

rakesh9177 commented Dec 30, 2023

Thank you for the review and tips @SagiPolaczek !

There are two ways to add readme file to rst file.

1)Manually add text like ".. mdinclude:: ../../fuse/dl/README.md" to rst files
2)To do an os.walk() and it will search for README file and then add to to their respective rst files.

I am not sure why we would want to delete the below code because it is being appended to rst files when doing and os.walk()
INCLUDE_TEXT = ".. mdinclude:: "

But If we want to use approach 1, we can remove all that code in conf.py which automatically links Readme to rst files.

Thanks!

@SagiPolaczek
Copy link
Collaborator

@rakesh9177 My bad, I wasn't clear enough - I meant to delete the comment(s) and the actual code. Thanks for verifying that :) That's not a must, I just like don't like artifacts from previous changes.

And thanks for the explanation! That's clear now.

Copy link
Collaborator

@SagiPolaczek SagiPolaczek left a comment

Choose a reason for hiding this comment

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

Awesome @rakesh9177 , thanks again!

Lets wrap it up with the following:

  1. Uncomment the function in conf.py (again, my bad for not being clear enough).
  2. Make sure it works OK. (I'll also try to rebuild the entire docs once it's done).

After those we'll be ready to merge.

Thanks again and wish you an happy new year!

docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
@SagiPolaczek
Copy link
Collaborator

Thanks @rakesh9177 !
I review the changes and it looks good to me! I'll approve and merge your PR later on today. Please let me know if you planned any extra change.

FYI, I've opened an account on readthedocs, but before launching the repo's website I need to get permissions from my org admin.

@rakesh9177
Copy link
Contributor Author

Great @SagiPolaczek , I have no plans to add extra changes. Thank you!

Copy link
Collaborator

@SagiPolaczek SagiPolaczek left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@SagiPolaczek
Copy link
Collaborator

@rakesh9177 Thanks for your contribution :)

@SagiPolaczek SagiPolaczek merged commit e91e0c5 into BiomedSciAI:master Jan 8, 2024
5 checks passed
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