-
Notifications
You must be signed in to change notification settings - Fork 16
switch to dep groups (PEP 735) #145
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
base: main
Are you sure you want to change the base?
switch to dep groups (PEP 735) #145
Conversation
…2 updates Bumps the actions group with 2 updates in the / directory: [actions/checkout](https://github.com/actions/checkout) and [actions/setup-python](https://github.com/actions/setup-python). Updates `actions/checkout` from 5.0.0 to 6.0.0 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@08c6903...1af3b93) Updates `actions/setup-python` from 6.0.0 to 6.1.0 - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@e797f83...83679a8) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 6.0.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions - dependency-name: actions/setup-python dependency-version: 6.1.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions ... Signed-off-by: dependabot[bot] <support@github.com>
This reverts commit 3bef2e1.
|
okay so since tests do pass, I tried to address the "hack" with twine with this: and can report that this did not work. like... where else would this go?
still: really feels like a broader issue. what did work was: # Explicit dependencies are required as a workaround for dependency-groups
# not being installed in builder environments (see https://github.com/pypa/hatch/issues/2152)
# This ensures twine is available when scripts run
dependencies = [
"twine",
] |
|
TBH I don't actually have the mental capacity at this moment to decide if 77e0622 is actually the right thing to do (or if it's better to revert it), I'll let someone else make that call. my understanding of the acceptance criterion was there may be a better solution here. cc @sneakers-the-rat do you have an opinion on this matter? |
| {%- endif %} | ||
| Download = "https://pypi.org/project/{{ project_slug }}/#files" | ||
|
|
||
| {%- if not use_hatch_envs %} |
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.
@mathematicalmichael optional-dependencies are for optional features in a package. so they shouldn't be correlated with development-groups which are for development dependencies. I suggest we still include them/remove the conditional here and just add a comment about how they are used.
| {%- if not use_hatch_envs %} |
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.
i'm trying to find the end of this condition - but we may just need to move it down.
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.
line 128
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.
thanks!! yes i think project.optional-dependencies can just be empty in our pyproject.toml template with a comment that the user can use that for optional features if they have them. but it shouldn't correlate to hatch environments because it's just a core pyproject.toml feature and development groups just allow us to separate feature dependencies from development.
Please let me know if i'm missing your intention here!
| # The groups below should be in the [development-groups] table | ||
| # They are here now because hatch hasn't released support for them but plans to | ||
| # in Mid November 2025. | ||
| # When not using hatch environments, we keep optional-dependencies for backward compatibility |
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.
| # When not using hatch environments, we keep optional-dependencies for backward compatibility | |
| # Optional-dependency table can be used to features that support optional functionality in your package. Remove this section if you don't have optional features in your package! |
| # They are here now because hatch hasn't released support for them but plans to | ||
| # in Mid November 2025. | ||
| # When not using hatch environments, we keep optional-dependencies for backward compatibility | ||
| dev = [ |
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.
I think this entire section can get moved to development groups but i may be wrong! it's hard to wrap my head around a jinja template in a pr!! but dev definitely belongs in development-groups not optional-dependencies
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.
yeah they are really annoying to review w/o indents, aren't they?
there's already an equivalent dep group for dev on 137 - so yeah perhaps this can get yeeted / was just left over.
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.
they are!!
| # Explicit dependencies are required as a workaround for dependency-groups | ||
| # not being installed in builder environments (see https://github.com/pypa/hatch/issues/2152) | ||
| # This ensures twine is available when scripts run | ||
| dependencies = [ | ||
| "twine", | ||
| ] |
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.
I need to test this again - the pr to fix this was merged, but when I tested this last, it worked as long as builder was set to true in the environment definition here. Let me do a sanity check on it - @mathematicalmichael, did you run through the template and try it with Hatch 1.16.2 without adding twine explicitly? I'm sorry if I missed a discussion of this, but I thought it worked, but you just had to make it a builder envt. it's possible i just had the newest commit/fix installed and didn't realize it but i thought that was the patch for now - builder=true.
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.
yup. in commit history i tried to get rid of twine explicitly a few times and it kept failing. the only other workaround was to pip install it outside (when installing hatch) - equally bad.
i only pushed up stuff that passed locally to CI. everything else failed.
lwasser
left a comment
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.
@mathematicalmichael thank you so much for this pr. I will test this - but likely not until next week after the holiday monday. I left a few comments but i want to redo my tests because i've installed various versions and commits where hatch was updated so i want to do a sanity check on what works and what doesn't first!
| # Explicit dependencies are required as a workaround for dependency-groups | ||
| # not being installed in builder environments (see https://github.com/pypa/hatch/issues/2152) | ||
| # This ensures twine is available when scripts run | ||
| dependencies = [ |
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.
this irks me
I agree. I was actually thinking about leaving that comment! At a minimum we want 1.16.2 where dependency group support was added. ✨ |
I think
main.ymlshould be more explicit aboutpython -m pip install hatchas 1.16+ is definitely required. but I left it be for now.I ran with
uv run --with hatch --with twine hatch...in order to test this, but ultimately want to open the PR to see if CI/CD will pass as well.it bothers me that
twineneeded to be added tomain.ymlbut 3bef2e1 didn't address it like I expected. somehowhatchis not properly handling the dep group for itself?If there's one "hack" "in this PR, that feel like it. perhaps
twinebelongs elsewhere.fixes #143