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

Sphinx documentation #153

Merged
merged 14 commits into from
Nov 14, 2024
Merged

Sphinx documentation #153

merged 14 commits into from
Nov 14, 2024

Conversation

wigging
Copy link
Collaborator

@wigging wigging commented Oct 24, 2024

This adds Sphinx for generating documentation for the package. The Furo theme is also added for a more modern design and layout of the documentation pages. The associated dependencies are added to the optional docs group in the pyproject.toml file.

@wigging wigging self-assigned this Oct 24, 2024
@wigging
Copy link
Collaborator Author

wigging commented Oct 24, 2024

I will rebase this with the dev branch after the other pull request has been merged. The number commits for this pull request will be much lower after the rebase.

@wigging wigging force-pushed the sphinx-docs branch 2 times, most recently from 52d85f4 to d5aac1a Compare October 31, 2024 20:06
@wigging wigging marked this pull request as ready for review November 1, 2024 19:45
@wigging
Copy link
Collaborator Author

wigging commented Nov 1, 2024

Since there are basically no docstrings in flowcept there isn't much documentation to generate. This will have to be improved with future pull requests. Read The Docs also needs to be setup so people can actually view the documentation but that can be done later too.

@renan-souza
Copy link
Collaborator

@wigging Is this PR ready to review?

Wrt adding docstrings to FlowCept, there is an open issue related to this: #50
We can benefit from a meeting to better explain how FlowCept is expected to be used and extended.

Perhaps we could work on it after we merge this PR.

@wigging
Copy link
Collaborator Author

wigging commented Nov 4, 2024

Looks like I need to update the CONTRIBUTING.md with info on how to build the docs. I'll do that soon.

@wigging
Copy link
Collaborator Author

wigging commented Nov 6, 2024

@renan-souza I added a few lines to the contributing document. I'll focus on resolving that issue in a separate pull request.

@renan-souza
Copy link
Collaborator

Looks like I need to update the CONTRIBUTING.md with info on how to build the docs. I'll do that soon.

Since you are changing it in this PR, do you mind adjusting the text that says we use black/flake for code formatting?

@wigging
Copy link
Collaborator Author

wigging commented Nov 6, 2024

Black and Flake8 are not used in this project.

@renan-souza
Copy link
Collaborator

renan-souza commented Nov 6, 2024

Black and Flake8 are not used in this project.

Exactly. But the CONTRIBUTING says they are. Since you are updating the CONTRIBUTING, can you please update the text in it where it mentions those and update with what we are currently using for code formating?

@wigging
Copy link
Collaborator Author

wigging commented Nov 6, 2024

Cleaned up the contributing guidelines and added more info about code linting and formatting.

@wigging
Copy link
Collaborator Author

wigging commented Nov 7, 2024

I added a Makefile in the root directory of the project to help with various commands such as running checks, running tests, starting containers, etc. This is especially helpful for Docker commands which I can never remember.

@wigging
Copy link
Collaborator Author

wigging commented Nov 7, 2024

@renan-souza I don't have anything else to add to this pull request.

@renan-souza
Copy link
Collaborator

@renan-souza I don't have anything else to add to this pull request.

Sounds good. I will start reviewing it and add new commits if needed.

Makefile Show resolved Hide resolved
@renan-souza
Copy link
Collaborator

@wigging Before we merge this PR, I would feel more comfortable if we tested the sphinx docs procedure.

Do you mind adding a new CI workflow based on https://github.com/ORNL/flowcept/blob/sphinx-docs/.github/workflows/run-checks.yml to test if the documents are being generated correctly?

This workflow can run only on PRs, just like the run-checks.yml.

Also, you could create a new command in Flowcept's Makefile to generate the documents.

Once we put sphinx as part of the CI process and it passes the test, I'll merge & close this PR.

@wigging
Copy link
Collaborator Author

wigging commented Nov 12, 2024

It builds fine on my local computer but I'll add something to the GitHub Actions to check the build too. And Sphinx already provides a Makefile (see docs/Makefile) for building the documentation and doing other things.

@renan-souza
Copy link
Collaborator

renan-souza commented Nov 12, 2024

It builds fine on my local computer but I'll add something to the GitHub Actions to check the build too.

Thanks. The goal is to increase the test coverage of all these features we add into the project.

And Sphinx already provides a Makefile (see docs/Makefile) for building the documentation and doing other things.

Yea, I saw that, that's fine, but since we now have our own Makefile, it's more convenient to use that same Makefile for everything rather having multiple Makefiles and cd into a directory to run it.

@wigging
Copy link
Collaborator Author

wigging commented Nov 12, 2024

Yea, I saw that, that's fine, but since we now have our own Makefile, it's more convenient to use that same Makefile for everything rather having multiple Makefiles and cd into a directory to run it.

You can pursue this in a separate pull request. This pull request is about setting up Sphinx, not fiddling around with Makefiles. The Makefile in the docs directory is generated by Sphinx and it uses the underlying sphinx CLI commands. Those commands expect to be run from the root level of the docs directory. I don't know what all these commands are but you will have to avoid naming conflicts if you do a recursive Makefile. You also would need to properly handle the path to the sphinx config file and other files if you try to use a top-level Makefile. Just cd into the docs directory and use the Sphinx Makefile as intended.

@wigging
Copy link
Collaborator Author

wigging commented Nov 13, 2024

@renan-souza Did something change on the dev branch to cause the tests to fail? None of the changes in this pull request should affect the tests.

@renan-souza
Copy link
Collaborator

@renan-souza Did something change on the dev branch to cause the tests to fail? None of the changes in this pull request should affect the tests.

Nothing has changed that could cause that error. Not sure what happened. I'll need to debug.

@wigging
Copy link
Collaborator Author

wigging commented Nov 13, 2024

All of the errors and test failures seem to be caused by code that uses Dask.

@renan-souza
Copy link
Collaborator

renan-souza commented Nov 13, 2024

All of the errors and test failures seem to be caused by code that uses Dask.

Yes and nothing about Dask has changed on our side.
Since the CI always installs the newest version of these dependencies, I suspect that something changed in the newest version of Dask that caused this error.

Please ignore these Dask errors as they have nothing to do with your chages.

I'll need to dig into this later.

@wigging
Copy link
Collaborator Author

wigging commented Nov 13, 2024

I went ahead and added the Makefile commands to build/clean the Sphinx docs from the top-level of the project. I removed the Makefile in the docs directory. I also added the Makefile commands in the run-checks workflow.

@wigging
Copy link
Collaborator Author

wigging commented Nov 14, 2024

@renan-souza Anything else needed here?

@renan-souza renan-souza merged commit 8668fff into dev Nov 14, 2024
3 checks passed
@renan-souza renan-souza deleted the sphinx-docs branch November 15, 2024 17:58
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