Skip to content

Conversation

FoamyGuy
Copy link
Contributor

patch that adds sphinx-rtd-theme into docs/requirements.txt which is needed since a change in RTD behavior starting 10/3 outlined in this post: https://blog.readthedocs.com/defaulting-latest-build-tools/

For our usage the crux is that sphinx-rtd-theme used to be installed automatically, but no longer is so we need to specify it in a requirements file in order for it to be installed moving forward.

@FoamyGuy
Copy link
Contributor Author

I started wondering why the actions CI were able to succeed in Github since we hadn't previously included sphinx-rtd-theme in a reqs file. It turns out that our actions has a step that does the install: https://github.com/adafruit/workflows-circuitpython-libs/blob/476e77fb0a3acd5aeaafaa943cc0dbf4e9fa3bcf/build/action.yml#L55

Theoretically it could be removed from the actions task if the change from this PR patch is adopted as long as something installs docs/requirements.txt then it'll get the theme. Though I don't think installing it twice should hurt anything either.

@FoamyGuy FoamyGuy requested a review from a team October 23, 2023 14:01
Comment on lines 19 to 22
--
--
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed this: something, probably pre-commit, removed a space after --. The special sequence -- has particular meaning in a git patch file. pre-commit needs to be instructed not to run the trailing whitespace on "*.patch" files, probably with an exclude directive, see under https://pre-commit.com/#adding-pre-commit-plugins-to-your-project

Copy link
Contributor

Choose a reason for hiding this comment

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

       --[no-]signature=<signature>
           Add a signature to each message produced. Per RFC 3676 the
           signature is separated from the body by a line with '-- ' on it. If
           the signature option is omitted the signature defaults to the Git
           version number.

or you could instruct git format-patch to not produce the "signature" part

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you get a chance to respond to this review comment? if so, I missed it

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 didn't see any messages on this page until coming back to it now. Weird, I'm not sure why it wasn't showing me the comments from last week.

I'll make one with that no signature flag and commit the one that it produces. Thank you

sphinx>=4.0.0
sphinxcontrib-jquery
+sphinx-rtd-theme
--
Copy link
Contributor

Choose a reason for hiding this comment

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

This trailing space needs to remain, otherwise the file is not a proper git patch. Pre commit needs to be told to leave it. I left a review with more detail last week but it's still "pending" and I can't figure out how to finish it from phone. Sorry for the lack of detail

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I guess it posted them all at once. Doh.

@FoamyGuy
Copy link
Contributor Author

Nice catch and thanks again Jeff!

Pre-commit was innocent in this instance. The culprit was my IDE, there is a setting that automatically strips trailing spaces in some situations. It took effect when I copied the patch file into the project and then saved it. I hadn't realized it was doing that before, but I've disabled it now.

The latest commit adds back in the space. After thinking about it more I decided against the --no-signature patch, I figured there is some chance that the version of the git client that produced the patch may be helpful to know some day.

Copy link
Contributor

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks!

@jepler jepler merged commit e55f7f5 into adafruit:main Nov 27, 2023
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