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

[Refactor] Migrate dependencies in docs/requirements.txt to pyproject.toml #325

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

kenibrewer
Copy link
Member

@kenibrewer kenibrewer commented Sep 29, 2023

Description

This migrates the dependencies for the docs build process into the pyproject.toml file to unify all dependency installs using Poetry. It also adds a docs preview pipeline that adds a preview link to any newly created PRs.

Fixes #321

What is the nature of your change?

  • Bug fix (fixes an issue).

Checklist

Please ensure that all boxes are checked before indicating that a pull request is ready for review.

  • I have read the CONTRIBUTING.md guidelines.
  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have deleted all non-relevant text in this pull request template.

@kenibrewer
Copy link
Member Author

@d33bs Is there a way to test the docs build process for this pull request? I'm not sure where to look for that.

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2023

Codecov Report

Merging #325 (ad01a81) into main (74e6c63) will increase coverage by 0.22%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #325      +/-   ##
==========================================
+ Coverage   95.40%   95.62%   +0.22%     
==========================================
  Files          56       56              
  Lines        3134     3134              
==========================================
+ Hits         2990     2997       +7     
+ Misses        144      137       -7     
Flag Coverage Δ
unittests 95.62% <ø> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kenibrewer kenibrewer marked this pull request as ready for review October 2, 2023 16:22
@d33bs
Copy link
Member

d33bs commented Oct 2, 2023

Hi @kenibrewer, previously I created a standalone Read the Docs deployment for testing via a fork here: https://readthedocs.org/dashboard/pycytominer-test-docs . I'd be glad to invite you to administrate this and use for testing if you'd like. Generally, I'd propose we move into a formalized testing process for documentation to cover scenarios like this one and others as they come up. Would you have any thoughts on the approach here?

@kenibrewer
Copy link
Member Author

@d33bs I think we should configure the Pull Requests features for Read the Docs here described here. That should allow us to use just one ReadTheDocs deployment instead of two.

With that PR setting turned on, we should be able to use the Github action here, to attach previews to any PR that affects docs.

@kenibrewer kenibrewer marked this pull request as draft October 4, 2023 15:23
@kenibrewer kenibrewer marked this pull request as ready for review October 4, 2023 16:15
@d33bs d33bs self-requested a review October 6, 2023 15:23
Copy link
Member

@d33bs d33bs left a comment

Choose a reason for hiding this comment

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

Great stuff with this PR, thank you for sending this through! I left a minor comment about naming for Github Actions, otherwise I think things are looking good.

Regarding your previous thoughts about the approach, I love the idea! Would you be interested in helping to enable the functionality on Readthedocs via the proper access there? Please let me know and I'll add you so you may configure things appropriately. If not, I can help out with the adjustments which are needed.

Either way, once we enable the functionality, do we need to do anything special to see the results in this PR?

.github/workflows/docs.yml Outdated Show resolved Hide resolved
@kenibrewer
Copy link
Member Author

Regarding your previous thoughts about the approach, I love the idea! Would you be interested in helping to enable the functionality on Readthedocs via the proper access there? Please let me know and I'll add you so you may configure things appropriately. If not, I can help out with the adjustments which are needed.

Sure! I'd be happy to figure that integration on ReadTheDocs. I think there might be an additional solution I might be able to enable if I'm a maintainer for both the Read The Docs project and Github using the ReadTheDocs Github app. That option should show the actual ReadTheDocs build process along with all the other pipelines defined with Github Actions.

Either way, once we enable the functionality, do we need to do anything special to see the results in this PR?

As currently set up, the workflow only runs when the pull request is created which is what is suggested in the ReadTheDocs Action documentation. I tried enabling it to run on any pull request event, but I interpreted "Resource Not Available" error to mean that the event is designed only to run on PR creation because that's when it has permissions to edit the PR description.

One corner case I'm aware of, is that if someone creates a PR without docs changes and then adds them later, this preview pipeline will never run. We could enable the pipeline to run on all PRs instead by getting rid of the paths filter.

In the end though, the docs preview pipeline is a (useful) convenience function that will help contributors know where they can preview any changes they make to the docs. I think the full ReadTheDocs Github App integration is the workflow that will prevent us from merging any changes that break the docs build process.

@kenibrewer kenibrewer merged commit 9d084b0 into cytomining:main Oct 11, 2023
10 checks passed
@kenibrewer kenibrewer deleted the kenibrewer/issue321 branch October 11, 2023 13:59
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.

[Refactor] Migrate dependencies in docs/requirements.txt to pyproject.toml
3 participants