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

Documentation #147

Merged
merged 7 commits into from
Jan 2, 2025
Merged

Documentation #147

merged 7 commits into from
Jan 2, 2025

Conversation

yuki462-b
Copy link
Contributor

What this PR does / why we need it

I have created a documentation for oper8.

It currently has descriptions of functions, classes, return values and input types. And they can be searched via UI.

Those are generated from docstrings, and the build is also automated with github workflow, so all the dev need is to write some docstrings, and merge codes into main branch, then the document should be updated automatically.

Documentations are written under docs folder.

  • docs/API References.md
  • docs/index.md

And github workflow is added to automatically publish the documentation to GitHub pages.

  • .github/workflows/publish-document.yml

Currently it is not published yet. In order to view the documentation locally, please follow [Optional] Documentation setup at CONTRIBUTING.md. Below are the screenshots of the documentation.

image image image image

The following tools are used:

Special notes for your reviewer

I would like to publish this documentation using github pages, but it seems I do not have necessary permission to do so. The setting is written at .github/workflows/publish-document.yml, so please setup the github pages if you can.

Also, feel free to update the documentation. I think we need something like getting started page to give user some simple examples to show how to use oper8. I would like to write this page by myself, but to be honest, I don't understand this library that well ..., so someone please make the page, or give me some advice 🙏.

If applicable**

  • this PR contains documentation
  • this PR has been tested for backwards compatibility

What gif most accurately describes how I feel towards this PR?

Example of a gif

Copy link
Collaborator

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

Thank you so much for adding this @yuki462-b! I've got a few suggestions around the automation and dependency tracking, but this is a great start for getting more visibility into the codebase.

on:
push:
branches:
- master
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need master in here

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 agree. Removed with this commit 89b0527.

mkdocs-material-
- name: Install MkDocs Dependencies
run: |
pip install mkdocs-material
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would great to put all of this into pyproject.toml as an additional dev dependency set and then add a script in scripts to run the generation locally. This would allow us to dry-run docs easily locally which would be really useful if this docs section grows beyond auto-generated API references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice!

I have updated pyproject.toml, tox.ini and added a shell script scripts/document.sh. Now, user can just run tox -e docs to build the current documentation, or tox -e docs -- serve to view the docs locally. 457dbf4

.github/workflows/publish-document.yml also uses tox command to publish the documentation. 4b27576

@@ -26,3 +26,6 @@ venv/

# Setuptools scm version
_version.py

# mkdocs documentation
/site
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we want to remove the leading / since I'm guessing this absolute path is only correct if developing inside a docker container

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 sorry, but I don't understand which docker container you are mentioning.

I do not use docker container for my development environment. I simply followed CONTRIBUTION.md, and it does not have any description for docker.

And when I build the document with mkdocs build command, it outputs the document into /site folder, so I updated gitignore to ignore this directory.

Is there a way to create the development environment with docker?


If you want to manually edit the document, please run the following commands to setup the editing environment.

```bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do go the route or scripting and tracking deps in pyproject this will need to be updated

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 don't think we have to chose this route. Please check #147 (comment)

@yuki462-b
Copy link
Contributor Author

@gabe-l-hart
Thank you for the review. I have addressed all of your comment, so could you take a look again when you have time?

Copy link
Collaborator

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

Ship it! This looks great

@gabe-l-hart gabe-l-hart merged commit d2e3e00 into IBM:main Jan 2, 2025
6 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