-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add inline_comment_prefixes
to ConfigParser
#3330
Conversation
Hi @jaraco, do you have an opinion about this change? Although the file format is very much ad-hoc, it seem to be fairly natural that users would just assume that inline comments can be used in a |
Changed ``ConfigParser`` instantiation to strip inline comments during the parsing | ||
of ``setup.cfg`` files. | ||
To prevent problems with URL values, users are expected to use at least one | ||
space character to separate the option value from the comment marker ``#``. |
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.
Is there any chance this change is disruptive enough to justify classifying it as a "breaking change"?
Is there anyone relying on the fact that setuptools does not strip inline comments?
I plan to merge this change soon. |
"""Make the ConfigParser forget everything (so we retain the original filenames | ||
that options come from). Private API for internal setuptools use. |
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.
nitpick:
"""Make the ConfigParser forget everything (so we retain the original filenames | |
that options come from). Private API for internal setuptools use. | |
"""Make the ConfigParser forget everything (retain the original filenames | |
from options). Private API for internal setuptools use. |
I'd like to see verification of the original claim. I'll apply the test to a commit prior to 62.3 and it should pass there, but fail after merging with 62.3. |
Given that pyproject.toml is preferred and since we've determined that this wasn't a regression, but just a feature request, I'd be okay to decline the change and just ask them not to use trailing comments. Your call. |
Thank you very much @jaraco for the in-depth investigation (sorry for not checking it before). I agree with you! |
I'd consider this a breaking change, as it will affect other fields in setup.cfg that may have had legitimate content with " #". In fact, it makes me wonder if this might be a serious breaking change. What if someone wants to legitimately include these symbols in their long-description? e.g. "description: The world's |
According to a recent an error report in #3322, prior to version 62.3 it would seem that
setuptools
could accept inline comments insetup.cfg [metadata] version
, for example:This was actually surprising for me because
setuptools
simply instantiateConfigParser
with no argument, which means inline comments are not stripped during the parsing. Moreover as shown in the diff between 62.2 and 62.3, no code path used for handlingversion
seems to have been changed (with the exception of the update of thepyparsing
dependency).I really don't understand how the changes in v62.3 could have prevented inline comments from being stripped (v62.3 is mostly about adding warnings and seems completely unrelated), but the changes proposed in this PR seem to be the most appropriated way of handling inline comments (iif we want to...).
Summary of changes
inline_comment_prefixes=(" #", "\t#")
toConfigParser
instantiation."#"
(single character) because URL values can have fragments (which start with#
).Closes #3322
Pull Request Checklist
changelog.d/
.(See documentation for details)