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

Fix dependencies on 'terminal' plugin #5138

Merged
merged 1 commit into from
Apr 17, 2019
Merged

Fix dependencies on 'terminal' plugin #5138

merged 1 commit into from
Apr 17, 2019

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Apr 17, 2019 •
edited
Loading

Fix #5139

@ikonst ikonst force-pushed the notify_exception_without_terminal branch from 417e1bb to 24fac2e Compare April 17, 2019 08:27
@codecov
Copy link

codecov bot commented Apr 17, 2019 •
edited
Loading

Codecov Report

Merging #5138 into master will increase coverage by <.01%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5138      +/-   ##
==========================================
+ Coverage   96.06%   96.06%   +<.01%     
==========================================
  Files         114      114              
  Lines       25784    25790       +6     
  Branches     2550     2550              
==========================================
+ Hits        24770    24776       +6     
  Misses        704      704              
  Partials      310      310
Impacted Files Coverage Δ
src/_pytest/reports.py 95% <ø> (ø) ⬆️
src/_pytest/config/__init__.py 94.4% <100%> (ø) ⬆️
testing/test_config.py 99.49% <100%> (ø) ⬆️
src/_pytest/nodes.py 95.58% <66.66%> (ø) ⬆️
src/_pytest/python.py 93.3% <66.66%> (ø) ⬆️

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 008d043...d67d68f. Read the comment docs.

@ikonst ikonst changed the title Thanks for submitting a PR, your contribution is really appreciated! Fix dependencies on 'terminal' plugin Apr 17, 2019
@blueyed
Copy link
Contributor

blueyed commented Apr 17, 2019

Thanks for working on this!

I wonder what @RonnyPfannschmidt has to say about the terminal plugin options being coupled like that - shouldn't only the terminal part/plugin itself know about it in the best case?

@ikonst
Copy link
Contributor Author

ikonst commented Apr 17, 2019

I agree this coupling is not optimal. I only corrected the code with the assumption that this coupling is a necessary evil for now.

@blueyed
Copy link
Contributor

blueyed commented Apr 17, 2019 •
edited
Loading

@ikonst
That is good, yeah - just wondered myself how to improve this in the future.

@blueyed
Copy link
Contributor

blueyed commented Apr 17, 2019 •
edited
Loading

Since this is green on CI now, please squash and force-push it (it does not need 7 commits, and we have the squash-merge button disabled here).

@ikonst ikonst force-pushed the notify_exception_without_terminal branch from 8e53d50 to d67d68f Compare April 17, 2019 18:19
@blueyed blueyed merged commit e87d3d7 into pytest-dev:master Apr 17, 2019
@blueyed
Copy link
Contributor

blueyed commented Apr 17, 2019

Thank you!

@ikonst ikonst deleted the notify_exception_without_terminal branch April 17, 2019 22:12
blueyed added a commit to blueyed/pytest that referenced this pull request Apr 27, 2019
@blueyed
Terminal plugin is not semi-essential anymore
6513301 
Thanks to pytest-dev#5138.
@blueyed blueyed mentioned this pull request Apr 27, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoddemus nicoddemus nicoddemus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

notify_exception crashes with -p no:terminal
3 participants
@ikonst @blueyed @nicoddemus