-
Notifications
You must be signed in to change notification settings - Fork 0
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 build workflow #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! These workflows are going to be very valuable. I've added a couple of comments/questions inline.
pyproject.toml
Outdated
@@ -75,6 +76,7 @@ exclude = ''' | |||
/( | |||
\.git | |||
| \.github | |||
| .tox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be tox.ini
?
poetry lock --no-update | ||
poetry install | ||
pip install black | ||
pip install isort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't Black and isort come with poetry install --with dev
?
poetry lock --no-update | ||
poetry install --with test | ||
pip install pytest | ||
pip install pytest-cov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't pytest and pytest-cov come with poetry install --with test
?
.github/codecov.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be in the project root?
Source: https://docs.codecov.com/docs/codecov-yaml#repository-yaml
Note: Let's remember to squash these commits at merge time. |
I was running flake8 locally and it was lining through .tox as well which was causing issues however, I realize now it wouldn't get pushed to GitHub so no need to keep it in.
Unfortunately, using poetry doesn't seem to work for our workflows. I know it is possible since I have seen others do it but I can't figure out why it doesn't work. Check out this commit I made on the flake8-workflow PR, the workflow that ran as a result of this changed failed because the command line black wasn't found. Have tried this without the
Yes it should! Fixed. Also, I added an artifact that gets produced when the docs build workflow runs so now, we can see all the updates directly through GitHub without having to fetch the code and build the docs ourselves. I'll demo this in tomorrow's meeting for further clarity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, let's remember to squash the commits
Description:
Added a workflow to build the sphinx docs and check for any errors in the build. These errors will be annotated automatically to the PR.
Related Issues:
#16
Tips for the Reviewer:
Commit changes to docs build and observe workflow, if the changes break the build process there will be annotations appearing.
Also, we should probably update the readme.md or coding.md to guide contributors on what to do when a check fails (check annotations etc)
Checklist:
Please be sure to check all boxes honestly. This is to ensure a smooth development process, and to reduce the
likelihood of needing to make additional changes later on.
or whose authorship we cannot verify, may not be accepted.
I have verified that any updates to the project documentation are complete and look okay.
and isort style configurations.
pyproject.toml
and the Poetry lock filehave been updated to reflect any dependency changes.
contribution.
Note: Sorry for the polluted PR, unsure why that happened. There should only be the Docs build workflow here, the codecov one and flake8 should be in a separate PR and should not be merged in yet.