8000 Changes to make fresh project pass precommit and docs build by FoamyGuy · Pull Request #244 · adafruit/cookiecutter-adafruit-circuitpython · GitHub
[go: up one dir, main page]
Skip to content

Changes to make fresh project pass precommit and docs build #244

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 13, 2025
Merged

Changes to make fresh project pass precommit and docs build #244

merged 3 commits into from
Jan 13, 2025

Conversation

FoamyGuy
Copy link
Contributor

On a freshly baked library the sphinx docs fail with these warnings (treated as errors) about the api.rst file not containing a title

docs/index.rst:18: WARNING: toctree contains reference to document 'api' that doesn't have a title: no link will be generated [toc.no_title]

This change adds the title so that this error will not occur on new libraries.

@FoamyGuy FoamyGuy requested review from a team and removed request for a team January 10, 2025 20:40
@FoamyGuy
Copy link
Contributor Author

I think there may be a few more tweaks needed to get the resulting generated project to pass everything. I'll this out a few more times and make additional commits here once it's in a state where it passes out of the gate.

@FoamyGuy FoamyGuy mentioned this pull request Jan 13, 2025
Merged
@FoamyGuy FoamyGuy changed the title add title to docs/api.rst Changes to make fresh project pass precommit and docs build Jan 13, 2025
@FoamyGuy
Copy link
Contributor Author

Getting pre-commit to pass in all cases is a little bit tricky it turns out. Specifically choosing yes or no to adafruit_bus_device and adafruit_register causes whitespace that we have some control over, but not enough control to satisfy ruff in all the possible cases as far as I can tell.

This is the relavent section of the template:

cookiecutter-adafruit-circuitpython/{{ cookiecutter.__dirname }}/docs/{% if cookiecutter.sphinx_docs in ['y', 'yes'] %}conf.py{% endif %}

Lines 35 to 40 in 788309c

{% if cookiecutter.requires_bus_device in ["y", "yes"] -%}
"BusDevice": ("https://docs.circuitpython.org/projects/busdevice/en/latest/", None),
{%- endif %}
{% if cookiecutter.requires_register in ["y", "yes"] -%}
"Register": ("https://docs.circuitpython.org/projects/register/en/latest/", None),
{%- endif %}

With the code as it is now the case where bus device and register are both included work successfully and the resulting project files do pass pre-commit.

However if you choose no to bus device and register you end up with two empty lines in the generated file i.e.:

intersphinx_mapping = {
    "python": ("https://docs.python.org/3", None),


    "CircuitPython": ("https://docs.circuitpython.org/en/latest/", None),
}

Which ruff wants to remove.

As I understand it the liquid template language allows the options to either strip whitespace from before and/or after a template tag, or not strip whitespace before and/or after a template tag https://shopify.github.io/liquid/basics/whitespace/. But doesn't seem to offer a way to dynamically include the whitespace based on the condition evaluation i.e. include white space if condition was true, but strip it if condition was false.

Possibly it could work by making the logic more gnarly and making branches that cover each possible permutation of that intersphinx mapping dict and including the entire dict as needed within each branch. But that feels like a fairly unsatisfying solution since we then would have the dictionary definition duplicated 4 times for each possible permutation.

I'm wondering if we could use postgen hooks or something to trigger ruff to run on the resulting file(s) after they are generated.

@FoamyGuy
Copy link
Contributor Author

Okay, I was mistaken about Liquid templating engine, I think that is the one used by circuitpython.org perhaps and I got wires crossed. This repo uses Jinja2 which I am more familiar with specifically, but they are similar in a lot of ways including how their whitespace control works so the functional results are the same as described above.

I have added a commit that resolves this by using a larger branching if block where each possible permutation of intersphinx mapping has it's own branch with the full dictionary definition. This adds some relatively repetitive code and isn't the nicest to look at in this template file, but in the end this felt like a path of less resistance then adding the postgen hook to run ruff which came along with some of it's own hurdles.

I tested the current version of this PR branch after the latest commit and it now passes pre-commit under all 4 of the possibilities for req's

  • both bus_device and register
  • bus_device only
  • register only
  • neither bus_device nor register

@FoamyGuy FoamyGuy requested a review from a team January 13, 2025 17:33
tannewt
tannewt approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member
@tannewt tannewt 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!

@tannewt tannewt merged commit 7c2638f into adafruit:main Jan 13, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tannewt tannewt tannewt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FoamyGuy @tannewt
0