fix(manager/poetry): fix schema for optional dependencies #32096
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@sisp thanks for your PR. Do you have a test repo which can be used to demonstrate the before/after of this change? |
|
@rarkins Would you like to have just the repo with a minimal |
|
Ideally this would be a "full" type of repo to also check that nothing else broke, which also demonstrates the new functionality working (e.g. via a PR which wouldn't exist before). It's not necessary for it to be minimal because we won't be debugging it |
|
Sorry, I'm not 100% sure what you'd like to have. I can create a Python project on github.com that uses Poetry with extras. But how would you like Renovate (including this PR's changes) to run for this repo? |
|
I assume that your changes in this PR can result in PRs being created which weren't before? In that case, I was expecting:
|
|
Thanks for the clarification! I've created a test repo at https://github.com/sisp/renovate-test-poetry-extras and enabled the GitHub Renovate app which successfully created the PR sisp/renovate-test-poetry-extras#1, but when you check the log you'll see that Renovate assigns Renovate log for this branchThe problem wasn't a missing PR but the wrong |
rarkins
previously approved these changes
Oct 24, 2024
secustor
requested changes
Oct 26, 2024
…hort syntax tests
…optional = false|true` setting
6 tasks
|
Thanks for your review, @secustor! 👍 I've extended the test fixture While making these changes, I realized that the test fixture |
viceice
reviewed
Oct 27, 2024
Co-authored-by: Michael Kriese <[email protected]>
|
I've merged |
|
@secustor I've resolved the merge conflicts. |
secustor
approved these changes
Oct 30, 2024
|
🎉 This PR is included in version 38.138.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
6 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
I've fixed the Poetry schema for optional dependencies (aka extras) and their extraction logic to fix the
depTypevalue.Before this PR, Renovate incorrectly assumed Poetry extras to be declared like this:
The correct declaration is this:
That is, an extra is declared by the combination of a package with
optional = truein the[tool.poetry.dependencies]table and the assignment of the optional package to an extra in the[tool.poetry.extras]table.Since the
optional = false|trueattribute as well as its implications on Renovate'sdepTypevalue are common to several dependency types (PyPI, Git, path), I've added a schema mixin for optional dependencies, which setsdepType: 'extras'whenoptional = true, and intersected the relevant dependency schemas with this mixin. Further, I've extended thewithDepType()helper to support setting thedepTypeonly when its value has not been set to keepdepType: 'extras'for dependencies declared in the[tool.poetry.dependencies]table. ThedepTypevalues of dependencies declared in the (outdated)[tool.poetry.dev-dependencies]table as well as in[tool.poetry.group.<group>.dependencies]tables are force-overridden, as they don't supportoptional = false|true. That said, the dependency schema technically supportsoptional = false|truefor any dependency (i.e., also dev dependencies and dependency groups).I deliberately decided against creating separate schemas for dependencies in the
[tool.poetry.dependencies]table and for dependencies in the other tables for simplicity. Also Poetry's JSON schema at https://json.schemastore.org/partial-poetry.json takes this approach.The snapshot diff looks pretty ugly on GitHub because object order was changed, apart from that the actual change is always just this:
"managerData": { - "nestedVersion": false, + "nestedVersion": true, + "sourceName": undefined, },Context
I stumbled over Renovate's debug log for a Project using Poetry that showed an optional dependency having
depType: 'dependencies'and notdepType: 'extras', so I began investigating.Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via:
Snapshots were updated and look correct now.