Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle missing space with -p #4522

Merged
merged 1 commit into from Dec 11, 2018
Merged

Handle missing space with -p #4522

merged 1 commit into from Dec 11, 2018

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Dec 9, 2018

This still does not use an actual argument parser, which only gets
instantiated below, and it does not appear to make sense instantiating
it just for this pre-parsing it seems.

-p without the required value is being handled before already though,
so it could potentially be passed down from somewhere already?!

Fixes #3532.

@blueyed blueyed added type: bug problem that needs to be addressed topic: config related to config handling, argument parsing and config file labels Dec 9, 2018
@codecov
Copy link

codecov bot commented Dec 9, 2018 •
edited

Codecov Report

Merging #4522 into features will increase coverage by <.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #4522      +/-   ##
============================================
+ Coverage     95.75%   95.76%   +<.01%     
============================================
  Files           111      111              
  Lines         24922    25187     +265     
  Branches       2459     2515      +56     
============================================
+ Hits          23864    24120     +256     
- Misses          748      760      +12     
+ Partials        310      307       -3
Flag Coverage Δ
#docs 29.34% <61.11%> (+0.06%) ⬆️
#doctesting 29.34% <61.11%> (+0.06%) ⬆️
#linting 29.34% <61.11%> (+0.06%) ⬆️
#linux 95.58% <88.88%> (+0.01%) ⬆️
#nobyte 92.42% <88.88%> (-0.07%) ⬇️
#numpy 93.07% <88.88%> (-0.14%) ⬇️
#pexpect 42.18% <61.11%> (+0.51%) ⬆️
#py27 93.41% <88.88%> (-0.39%) ⬇️
#py34 91.93% <88.88%> (ø) ⬆️
#py35 91.96% <88.88%> (ø) ⬆️
#py36 91.97% <88.88%> (ø) ⬆️
#py37 93.74% <88.88%> (-0.19%) ⬇️
#trial 93.07% <88.88%> (-0.14%) ⬇️
#windows 93.97% <88.88%> (-0.06%) ⬇️
#xdist 93.74% <88.88%> (-0.12%) ⬇️
Impacted Files Coverage Δ
testing/test_pluginmanager.py 99.59% <100%> (ø) ⬆️
src/_pytest/config/__init__.py 93.96% <85.71%> (-0.24%) ⬇️
testing/test_pdb.py 96.49% <0%> (-2.5%) ⬇️
testing/test_cacheprovider.py 99.72% <0%> (ø) ⬆️
src/_pytest/terminal.py 92.95% <0%> (+0.83%) ⬆️
src/_pytest/cacheprovider.py 97.97% <0%> (+0.85%) ⬆️
src/_pytest/debugging.py 86.33% <0%> (+5.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 038f1f9...7b1cb88. Read the comment docs.

asottile
asottile approved these changes Dec 9, 2018
View reviewed changes
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

I wonder if we can leverage parse_known_args from argparse.ArgumentParser()

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
@blueyed
Copy link
Contributor Author

blueyed commented Dec 9, 2018

I wonder if we can leverage parse_known_args from argparse.ArgumentParser()

This gets used before.. pytest -p fails already.

nicoddemus
nicoddemus previously requested changes Dec 9, 2018
View reviewed changes
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

This should go in features I believe; while it seems like a bug, it arguably is an additional feature. IMO we don't want projects with addopts= -pmyplugin to depend on pytest's patch version.

changelog/3532.bugfix.rst Outdated Show resolved Hide resolved
@blueyed blueyed changed the base branch from master to features December 10, 2018 04:40
@blueyed blueyed dismissed nicoddemus’s stale review December 10, 2018 04:41

Rebased on features.

@blueyed
Handle missing space with -p
7b1cb88 
This still does not use an actual argument parser, which only gets
instantiated below, and it does not appear to make sense instantiating
it just for this pre-parsing it seems.

`-p` without the required value is being handled before already though,
so it could potentially be passed down from somewhere already?!

Fixes pytest-dev#3532.
nicoddemus
nicoddemus approved these changes Dec 11, 2018
View reviewed changes
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@blueyed
Copy link
Contributor Author

blueyed commented Dec 11, 2018

The patch appears to be covered 100% after all.

@blueyed blueyed merged commit ebe0a88 into pytest-dev:features Dec 11, 2018
@blueyed blueyed deleted the p-no-space branch December 11, 2018 12:33
@RonnyPfannschmidt RonnyPfannschmidt mentioned this pull request Dec 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt left review comments

@nicoddemus nicoddemus nicoddemus approved these changes

@asottile asottile asottile approved these changes

Assignees
No one assigned
Labels
topic: config related to config handling, argument parsing and config file type: bug problem that needs to be addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@blueyed @RonnyPfannschmidt @nicoddemus @asottile