Skip to content

Fix a small issue about shlex.split not working well with win32 path #1523

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

Merged
merged 3 commits into from
Apr 20, 2016

Conversation

afterhill
Copy link
Contributor

@afterhill afterhill commented Apr 19, 2016

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Target: Fix a win32 path issue, so I target master branch
  • Tests: added
  • Add yourself to AUTHORS (done)
  • Add a new entry to the CHANGELOG (done)

Issue description:

Under win32 machine:

import pytest
get_ini_path = from_some_other_func
pytest.main("-c " + get_ini_path)

Look at the shlex.split signature:

shlex.split(str, posix=True)

the posix default value is True, once I passed a win32 path (e.x. D:\tests\pytest.ini) into shlex.split, it will output as Dtestspytest.ini. This PR will check the platform firstly before applying the win32 path into shlex.split.

@@ -104,7 +104,10 @@ def _prepareconfig(args=None, plugins=None):
elif not isinstance(args, (tuple, list)):
if not isinstance(args, str):
raise ValueError("not a string or argument list: %r" % (args,))
args = shlex.split(args)
if sys.platform == "win32":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about just using shlex.split(args, posix=sys.platform == "win32")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RonnyPfannschmidt of course meant shlex.split(args, posix=sys.platform != 'win32'). 😉

@RonnyPfannschmidt
Copy link
Member

good find,

for platform-quicks, tests are necessary, since its easy to miss reasons and implementation details
i'm also wondering if we could perhaps just use https://docs.python.org/2/library/shlex.html#shlex.shlex.whitespace_split

@nicoddemus please have a quick look

@nicoddemus
Copy link
Member

@RonnyPfannschmidt sure, will do it on my lunch break later. 😁

@nicoddemus
Copy link
Member

Looks good, except for lack of tests as @RonnyPfannschmidt mentioned. 😁

Thanks @MengJueM for the work!

@afterhill
Copy link
Contributor Author

@RonnyPfannschmidt @nicoddemus One test is added.

@nicoddemus
Copy link
Member

@MengJueM while the change is small, I think it is worth a mention in the changelog... I'm sure more people has been bitten by it, so a changelog entry will help raise awareness that this has been fixed. 😁

Thanks again for the work!

@afterhill
Copy link
Contributor Author

afterhill commented Apr 20, 2016

@nicoddemus Changelog is added. I was getting impressed about the good quality and coverage when I started to review the testing cases of pytest. Thanks for your guys maintaining such a perfect project. Pytest is amazing.

@RonnyPfannschmidt RonnyPfannschmidt merged commit 6d661ac into pytest-dev:master Apr 20, 2016
@nicoddemus
Copy link
Member

Thanks, we do appreciate your compliment and the effort on the PR! 😄

nicoddemus added a commit to nicoddemus/pytest that referenced this pull request May 23, 2016
This fixes the bug inserted by accident in pytest-dev#1523
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants