-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Downgrade hyphen and uppercase deprecation warning to info #4923
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1 @@ | |||
This changes ``enforce_underscore`` and ``enforce_option_lowercase`` from deprecation warnings to information only. |
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 is supposed to read coherently in a change log.
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.
Ok, so how do you suggest I word this?
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.
How about,
Remove deprecation for dash-separated and uppercase fields in setup.cfg
?
If that is good, I'll patch that in.
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.
Well, that looks like a TODO-list item because it uses infinitive. It's usually present simple or the past tense, explaining what the impact is for the end-user.
Could you link a post where maintainers decided that this is acceptable? |
I am not sure this has been decided it is acceptable. It seems that there is not consensus based on the conversation in the issue, so I made this PR to clarify my meaning. If it is better to mark as draft, we can do that. |
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 very much @jamesliu4c for working on this but unfortunately this is a -1 from my side (other maintainers may have different opinions).
Before describing the reasons for the -1, I would like to clarify that with the benefit of the hindsight, it seems requiring _
for setuptools config fields is not really the core problem here. Based on the coarse feedback we received, if setuptools was dealing only with fields defined by itself, the impact of requiring _
do not seem very strong. The impression is that the community has sorted that one out. The crux of the problem is apparently #4913, which is something completely unforeseen when we studied what to do with the deprecation warning introduced many years ago.
(I am not saying that setuptools should necessarily go around failing for all fields it does not recognise, but that is a discussion in the context of #4913).
Now I apologise if I was not clear enough in my comment in #4921 (comment). I am (and I believe other maintainers would also be) are very happy to consider other alternatives that would solve the problem in a better way. But the alternative would need to be a better way of doing things, specifically:
- Can we rework the code in a way that it is simplified and at the same time can handle configurations with dashes?
- Can we change the code in a way that it seamlessly handle the dash x underscore dilemma and reduce the bloat in
setuptools.dist
? - Is there some innovative solution that drastically simplify this?
(all this questions are basically equivalent and I also had previously listed similar suggestions in #4921 (comment)).
Particularly it is also dear to me that we avoid having to read entry-points metadata with importlib
(and without having to hardcode or CTRL+C,CTRL+V the information of the existing commands that exists in pyproject.toml
1). I think that operation is too complex/costly if the intention is to simply accept dashes2.
While we are open to discuss and consider accepting -
, I think that it should come with a counterpart that also improves the codebase and/or performance (or at least a very compelling study of the state of the community and a convincing why the deprecation can not be handled by the packages holding down the change).
Footnotes
-
If I am not mistaken, in the past there was double bookkeeping of the command names, which is also not great, so it got appropriately replaced by metadata inspection to preserve a single source of truth. I don't think we want to revert that, but instead, obviate the need for it. ↩
-
I do need to clarify here that PR 4870 itself did not eliminate loop-ing through entry-points, but it was a step towards that direction. The initial idea was to give the community a very specific error easy to interpret and fix for a while before completely removing the convoluted mechanisms that identify the non compliant fields/sections (I believe that PR 4870 tided-up the previous implementation and potentially fixed some bugs, but alone it was not the final destination). ↩
Summary of changes
This changes
enforce_underscore
andenforce_option_lowercase
from deprecation warnings to information only.Closes
#4921
Pull Request Checklist
- [] Changes have testsnewsfragments/
.(See documentation for details)