-
Notifications
You must be signed in to change notification settings - Fork 2.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
Fix: allow whitespace in list of extras for a package #2985
Fix: allow whitespace in list of extras for a package #2985
Conversation
tests/console/commands/test_init.py
Outdated
def test_add_package_with_extras_and_whitespace(app): | ||
command = app.find("init") | ||
result = command._parse_requirements(["databases[postgresql, sqlite]"]) |
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.
def test_add_package_with_extras_and_whitespace(app): | |
command = app.find("init") | |
result = command._parse_requirements(["databases[postgresql, sqlite]"]) | |
def test_add_package_with_extras_and_whitespace(tester): | |
result = tester._parse_requirements(["databases[postgresql, sqlite]"]) |
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.
It look like it must be test._command._parse_requirements
...
@@ -373,7 +373,7 @@ def _parse_requirements( | |||
for requirement in requirements: | |||
requirement = requirement.strip() | |||
extras = [] | |||
extras_m = re.search(r"\[([\w\d,-_]+)\]$", requirement) | |||
extras_m = re.search(r"\[([\w\d,-_ ]+)\]$", requirement) |
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.
Shoud we just use \s
here?
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.
The range of \s
is to wide. I don't think supporting e.g. tab characters here.
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.
Hmm, I guess for this case it is fine (ish). For PEP 508, whitespace typically refers to "non-line breaking whitespace". That is narrowier than \s
and but wider than
. I doubt anyone is going to type in foo[a, \t b]
😏
56bc7f5
to
cddbcad
Compare
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.
Issue talks about add
command but the fix is in init
command?
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This fix allows having whitespaces in the list of extras during
poetry add
, e.g.poetry add databases[postgresql, sqlite]
.Pull Request Check List
Resolves: #2980