-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add dependency groups to pyproject.toml file #10332
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
Conversation
Pierre-Sassoulas
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.
Thank you for handling this ! I have some nits and some discussions of actual changes for other maintainers to chime in :)
|
Applied your suggestion, I think the failing pylint run seems like a caching issue to me, because it installs pylint but does not find the executeable. What do you think? |
DanielNoord
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.
Very cool! Thanks!
DanielNoord
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.
Assuming CI passes this LGTM!
|
We probably have to change the file based caching based on the requirement files too ? Not sure if the pipeline is failing because of the changes or because the cache is already broken. |
|
Yeah I think the cache is broken. I see that the pylint step is using pre-commit again. Maybe we should replace that? |
a971aaf
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10332 +/- ##
=======================================
Coverage 95.90% 95.90%
=======================================
Files 176 176
Lines 19108 19108
=======================================
Hits 18325 18325
Misses 783 783 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
for more information, see https://pre-commit.ci
Co-authored-by: Pierre Sassoulas <[email protected]>
Remove redundant comments Co-authored-by: Daniël van Noord <[email protected]>
This reverts commit a439b4d.
This reverts commit a971aaf.
This reverts commit 77b933b.
|
Ok, actually I have no idea why this fails here. |
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 39b9052 |
|
Found it! I accidentally deleted the entry point scripts and urls section when editing the Now the |
Pierre-Sassoulas
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.
Thank you for being so persistent @Julfried :)
|
Thanks for the PR. Please note though that dependabot does not support dependency-groups either. See dependabot/dependabot-core#10847. I tested it on my fork by reverting the typing-extensions bump. The PR dependabot created updates only On that note, it'll likely be a challenge to keep them in sync for the time being as the version requirements have already diverted. pylint/requirements_test_min.txt Line 4 in 70559b8
Line 106 in 70559b8
So, even while I'm in favor of moving to |
|
Ok I was not aware of the current limitation of dependabot. However since we still left the |
Yeah, it's probably not needed. However I think we should remember that the requirement files are the source of truth (at least for the time being). Mainly, so we don't update the CI pipeline just yet. As for dependabot PRs they will require more attention now. Alternative we could just ignore the dependency groups for now and don't keep them in sync but that's a case where I'd feel more comfortable reverting this PR for the time being.
Once the support is there, mainly in pip and dependabot, this should be fine. This will still take some time though as dependabot seems to move rather slowly. Maybe there is some more urgency there once the pip support is released. |
|
Thanks for the heads-up! Since CI and docs still rely on the requirements.txt files, I think we're fine for now. We can treat the dependency groups as optional until tooling catches up, and fully switch once support is there. That said, I’m also fine with reverting if you think it’s cleaner. What do you think @Pierre-Sassoulas? |
|
Let's revert and re-revert when the time is right. It's better if we deal with upgrading dependency group from the requirement only once during the re-revert. (Thank you for the review Marc, I wrongly assumed dependabot would 'just work') |
|
Sounds good, lets do that👍 |
This reverts commit 70559b8.
|
Reverted temporarily in #10342 |
Type of Changes
Description
Follow up to #10288. Introducing dependency groups in
pyproject.tomland cleaning up the file a little bit. Once pip fully supports PEP735, we should update the documentation and remove all therequirement.txtfiles.With this change you are able to install dependencies like this:
One thing I am currently unsure is how the dependencies in
.readthedocs.yamlwould be handled, when the requirement files in the doc folder are removed?