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.

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

skipping: factor out _get_pos, pass only config to _get_report_str #5002

Merged
merged 1 commit into from
Mar 27, 2019

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Mar 26, 2019

This is for "short summary".

I've started looking into making the format there configurable, to support e.g. "fname:lineno" instead of nodeids.

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #5002 into features will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #5002      +/-   ##
============================================
+ Coverage     95.97%   95.97%   +<.01%     
============================================
  Files           113      113              
  Lines         25359    25365       +6     
  Branches       2501     2501              
============================================
+ Hits          24339    24345       +6     
  Misses          714      714              
  Partials        306      306
Impacted Files Coverage Δ
src/_pytest/skipping.py 95.55% <100%> (+0.15%) ⬆️

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 ee96214...351529c. Read the comment docs.

1 similar comment
@codecov-io
Copy link

codecov-io commented Mar 26, 2019

Codecov Report

Merging #5002 into features will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #5002      +/-   ##
============================================
+ Coverage     95.97%   95.97%   +<.01%     
============================================
  Files           113      113              
  Lines         25359    25365       +6     
  Branches       2501     2501              
============================================
+ Hits          24339    24345       +6     
  Misses          714      714              
  Partials        306      306
Impacted Files Coverage Δ
src/_pytest/skipping.py 95.55% <100%> (+0.15%) ⬆️

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 ee96214...351529c. Read the comment docs.

@nicoddemus
Copy link
Member

I've started looking into making the format there configurable, to support e.g. "fname:lineno" instead of nodeids.

I'm curious, in order to jump to the code using your editor?

@blueyed
Copy link
Contributor Author

blueyed commented Mar 27, 2019

I'm curious, in order to jump to the code using your editor?

Yeah, fname:lineno is very common.

It was mainly inspired when looking at Teemu/pytest-sugar#173.

I am using a Vim pluging that handles pytest's nodeids also (https://github.com/wsdjeg/vim-fetch), but there then the lineno could be useful also.

I was thinking about changing the default to "nodeid, line X" at least - but allow for configuring this through an ini option, so that e.g. nodeid (fname:lineno) could also be used.

@nicoddemus
Copy link
Member

nicoddemus commented Mar 27, 2019

I was thinking about changing the default to "nodeid, line X" at least - but allow for configuring this through an ini option, so that e.g. nodeid (fname:lineno) could also be used.

I'm 👍 to change the default, but I don't think we need an option if that default is good enough for:

  1. Quickly selecting the node id to run again; for me this works if there's a space between nodeid and (fname:lineno);
  2. Configure your editor to automatically parse that line so you can jump to the code directly.
  1. is the main use case I see for today's behavior, and I believe it can get alongside 2 just fine.

What do you think?

@blueyed
Copy link
Contributor Author

blueyed commented Mar 27, 2019

I agree with not having unnecessary options, especially since this would involve a mini template engine in this case ("lineno" might be not available, and then you do not want "fname:" I guess).

I think we should retain nodeid, but having fname:lineno after it might make it very long (since the fname is basically duplicated then).
But I think just appending " (fname:lineno)" would be a good improvement nonetheless as a first step.

@nicoddemus
Copy link
Member

I think we should retain nodeid, but having fname:lineno after it might make it very long (since the fname is basically duplicated then).
But I think just appending " (fname:lineno)" would be a good improvement nonetheless as a first step.

Oh good point, it will probably be too long indeed...

@blueyed
Copy link
Contributor Author

blueyed commented Mar 27, 2019

So not a good default then?
It would be easier if pytest could handle fname:lineno for collection (then we could use just this here), but OTOH the nodeid is nice, and more stable (with changing files) in general.

I've also thought about extending the nodeid, i.e. fname::func:lnum, but this would e.g. not work with the Vim plugin I am using (but could be handled there, of course), or other tools handling only the old format.
Better would be fname:lineno::func as an extended nodeid, i.e. have :lineno optionally after the fname. Editors that support nodeids already could be extended to use this as an extra hint (or just ignore it, if they are looking for the test class/name; but it would be a good hint for when there are multiple / where to start), and also allows for using this with tools that only handle fname:lineno - with the drawback of not having the extra space there (for double click selection).

@blueyed blueyed merged commit 50a5ceb into pytest-dev:features Mar 27, 2019
@blueyed
Copy link
Contributor Author

blueyed commented Mar 27, 2019

Merging as-is already, happy to follow up the discussion here (or in a new issue, whatever you prefer).

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