-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PEP 771: Amendments following pre-publish discussion #4238
Conversation
…amples in each sub-section
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.
Brief review, I have only read up to the changes to Specification in depth.
Edit: Please can you add the linked thread to Post-History?
Post-History:
`15-Jan-2025 <https://discuss.python.org/t/77892/>`__,
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@AA-Turner - thanks for the review so far! I've added the |
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 for taking this on @astrofrog. I have several comments and questions after reading the PEP but not all of the DPO discussion. I agree with the Motivations, but I'm wrestling a bit if we're communicating the approach clearly enough. I'm not advocating changing the approach but making it more explicit for readers.
@willingc - thank you for your review! I will try and implement the comments by early next week. |
Co-authored-by: Carol Willing <carolcode@willingconsulting.com> Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>
@willingc - thanks again for the review! I've tried to implement your comments - there are a couple of remaining questions in some of the in-line discussions regarding whether any further text needs to be added or modified, so just let me know what you think. |
Skimming through this @astrofrog, I think that you have captured well the edits. 😄 I will take a look at the inline later tomorrow. Thank you! |
This branch is now out of date with main - should I just add a merge commit to fix that? (in the last PR I rebased but was told it wasn't ideal for reviews so just want to make sure I do the right thing) |
It's fine, there's no need to update from |
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.
Beautiful. Thanks @astrofrog.
Thanks for the approval! Once this is merged, I will open the main discussion thread on DPO and will open a follow-up PR updating the discussion link. |
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.
Any final comments from @pradyunsg? Otherwise let's merge.
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Nope. :) |
@AA-Turner @hugovk @willingc - I've made the thread and have added Discussion-To here - would you or someone else be able to merge shortly? Thanks! |
@AA-Turner - thank you for fixing and merging this! 🙏 (Sorry I didn't realize the thread also needed to go in Post-History) |
Packaging repository maintainers | ||
-------------------------------- | ||
|
||
The impact on individuals who repackage Python libraries for different | ||
distributions, such as `conda <https://docs.conda.io>`_, `Homebrew | ||
<https://brew.sh/>`_, linux package installers (such as ``apt`` and ``yum``) and | ||
so on, needs to be considered. Not all package distributions have mechanisms | ||
that would line up with the approach described. In fact, some distributions such | ||
as conda, do not even have the concept of extras. |
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.
Speaking from "the other side", I would have to say that pip functionality is a strict subset of the functionalities available by repackagers. Primarily based on my knowledge of linux package managers, but also with regard to conda and spack. I know you're specifically calling out conda here as not being able to support the PEP, but I'm saying conda specifically can -- and so can everyone else.
I feel like the python community by and large has a lot of expertise in how pip works but very little study into how anything other than pip works. :( There is a lot of interesting practical research that's been done over the decades. Perhaps you're already planning to do that though -- the wording of this section somehow gives me the feeling that it's a bit of an open TODO to flesh out the actual considerations! Feel free to ask me for more details if so.
(Incidentally: I oppose the PEP anyway because despite all the functionality of it being representable by repackagers, the PEP largely makes decisions that explicitly close the door on what I would consider as the primary functionality of "extras" as they are used in other ecosystems. There is very little to gain here without taking a holistic view of things. And much to lose if you e.g. define what is currently unspecified/invalid as newly-valid without adding new functionality using it. :P)
There has been an extensive discussion going on in this DPO thread while the original PR for PEP 771 was being reviewed. Rather than continuously update that PR, @pradyunsg suggested that that PEP should be published as-is initially and that changes based on the pre-publish discussion should be made in a separate PR. This is the separate PR 🎉
The main changes are:
default-optional-dependencies
key has been renamed todefault-optional-dependency-keys
Once this PR is merged, I'll then open the main discussion thread and open a trivial PR to add the link to the PR.
cc @pradyunsg @DEKHTIARJonathan
📚 Documentation preview 📚: https://pep-previews--4238.org.readthedocs.build/