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

Docs #28

Merged
merged 19 commits into from
Mar 7, 2023
Merged

Docs #28

merged 19 commits into from
Mar 7, 2023

Conversation

AhmetCanSolak
Copy link
Contributor

@AhmetCanSolak AhmetCanSolak commented Jan 19, 2023

This PR introduces documentation page to iohub. Description will be updated once draft status is removed from PR.

Please see docs/README.md for usage, right now you can locally build these docs and test. Any feedback is welcome. Should something like this:

image

@AhmetCanSolak AhmetCanSolak self-assigned this Jan 19, 2023
@ziw-liu
Copy link
Collaborator

ziw-liu commented Jan 19, 2023

Suggested theme: https://github.com/pydata/pydata-sphinx-theme

@ziw-liu ziw-liu added the documentation Improvements or additions to documentation label Feb 3, 2023
@AhmetCanSolak
Copy link
Contributor Author

I am not totally conviced by pydata-sphinx-theme, would like to take input from more people. Here how it looks with light and dark modes:
Screen Shot 2023-02-16 at 9 31 32 AM
Screen Shot 2023-02-16 at 9 32 10 AM

Here how readthedocs theme looks like for comparison:

Screen Shot 2023-02-16 at 9 32 49 AM

pydata theme doesn't build a sidebar from independent toctree elements automatically (but gives more configurability like being able to set independent sidebars for different pages) and I find it less browsable (for a complete doc example I think NumPy is a good one: https://numpy.org/doc/1.24/reference/index.html), so before I invest more time into it, I want to know what is the consensus on preferred theme. Open to new suggestions as well. Please give input @mattersoflight , @talonchandler , @keithchev , @JoOkuma

@keithchev
Copy link

I'm mostly agnostic. readthedocs theme looks a bit more readable to me, but using pydata would be consistent w established packages like numpy and zarr, which iohub is obviously closely related to, and this might help establish trust w external users.

@mattersoflight
Copy link
Collaborator

A sidebar is indeed useful to navigate and discover documentation. I am happy with any theme that provides sidebar like in numpy docs. The screenshot of the pydata-sphinx-theme GitHub page does show a sidebar.

@ziw-liu
Copy link
Collaborator

ziw-liu commented Feb 23, 2023

Hi @AhmetCanSolak do you have a preference/plan for deployment? For example GH pages vs readthedocs, and build workflows.

@AhmetCanSolak
Copy link
Contributor Author

yes @ziw-liu , I am planning to host on gh pages and already have gh actions tasks to deploy

@AhmetCanSolak
Copy link
Contributor Author

given pydata/pydata-sphinx-theme#221 I will move with readthedocs theme, I think not sacrificing brows-ability is important to us at this stage.

@ziw-liu
Copy link
Collaborator

ziw-liu commented Mar 1, 2023

given pydata/pydata-sphinx-theme#221 I will move with readthedocs theme, I think not sacrificing brows-ability is important to us at this stage.

Thanks for looking into this. For now I'm happy with what gets us to the docs pages the fastest.

@AhmetCanSolak AhmetCanSolak marked this pull request as ready for review March 1, 2023 19:06
docs/README.md Outdated Show resolved Hide resolved
@AhmetCanSolak AhmetCanSolak requested a review from keithchev March 1, 2023 21:13
@ziw-liu
Copy link
Collaborator

ziw-liu commented Mar 2, 2023

@AhmetCanSolak Can the documentation guidelines be added to CONTRIBUTING.md?

Copy link

@keithchev keithchev left a comment

Choose a reason for hiding this comment

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

Made a few minor but critical comments (note that I did not review the content of the docs but just the infra and build workflow).

docs/Makefile Outdated Show resolved Hide resolved
Comment on lines 29 to 32
working-directory: ./docs
run: |
python -m pip install --upgrade pip
pip install -r requirements-docs.txt
Copy link

@keithchev keithchev Mar 2, 2023

Choose a reason for hiding this comment

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

this step should use pip install .'[doc]' since the build deps are specified in setup.cfg and not in docs/requirements-docs.txt. Also, this obviously means this step's working-directory should not be changed to docs/.

aside: it's obviously a bit inefficient to install the iohub package just to install the docs-build deps, but I think it's okay unless/until the package deps become unwieldy or complex (which hopefully never happens).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch, addressed.

regarding your aside: AFAIK, it is actually required to install your package to utilize autosummary features the way we use with sphinx. Otherwise importing package submodules fails in different doc source files.

Choose a reason for hiding this comment

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

That does make sense, but I know that the copylot docs workflow does not install the package, just the build deps, and it works (unless its docs don't use the autosummary features? I know nothing about sphinx...)

Choose a reason for hiding this comment

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

Hmm I think this (ugly) line in the sphinx config might eliminate the need to install the package: sys.path.insert(0, os.path.abspath("../../"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not very welcoming to path manipulation idea :) but if majority agrees on it I can live with it.

Choose a reason for hiding this comment

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

I mean, it's already in there, and it works, so I would vote for leaving it alone

@keithchev
Copy link

Also, just for reference, after merging this PR we will need to make the following changes to the repo settings on github in order to allow the docs workflow to deploy the docs:

  • under "pages", set the source to "github actions"
  • under "environments", add a new environment called 'github-pages' and allow the main branch to deploy to it

@AhmetCanSolak
Copy link
Contributor Author

@AhmetCanSolak Can the documentation guidelines be added to CONTRIBUTING.md?

Just did it.

Copy link
Collaborator

@ziw-liu ziw-liu 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! Thanks @AhmetCanSolak

Copy link

@nbathreya nbathreya 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!

@mattersoflight
Copy link
Collaborator

Hi @AhmetCanSolak how can I locally build the docs and view? Do I need to setup a specific environment for documentation?

@ziw-liu
Copy link
Collaborator

ziw-liu commented Mar 7, 2023

addressed Ziwen's comment

Hi @mattersoflight, you can build locally following 42670af.

@AhmetCanSolak
Copy link
Contributor Author

Hi @AhmetCanSolak how can I locally build the docs and view? Do I need to setup a specific environment for documentation?

here: https://github.com/czbiohub/iohub/pull/28/files#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055

@AhmetCanSolak
Copy link
Contributor Author

@mattersoflight , I will merge this PR now but please provide your feedback whenever you can. I will make follow up PRs to address them.

@AhmetCanSolak AhmetCanSolak merged commit f5beac1 into main Mar 7, 2023
@AhmetCanSolak AhmetCanSolak deleted the docs branch March 7, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants