Skip to content

Move import of _format_explanation in _pytest.assertion.rewrite#4863

Merged
blueyed merged 1 commit into
pytest-dev:featuresfrom
blueyed:remove-import
Mar 1, 2019
Merged

Move import of _format_explanation in _pytest.assertion.rewrite#4863
blueyed merged 1 commit into
pytest-dev:featuresfrom
blueyed:remove-import

Conversation

@blueyed

@blueyed blueyed commented Mar 1, 2019

Copy link
Copy Markdown
Contributor

Moved/added there in 250160b, but apparently not intentionally?!

# noqa was only added later in two steps.

/cc @benjaminp

nicoddemus
nicoddemus approved these changes Mar 1, 2019
View reviewed changes

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If tests pass and @benjaminp agrees, LGTM!

@codecov

codecov Bot commented Mar 1, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #4863 into features will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #4863      +/-   ##
============================================
- Coverage     95.75%   95.65%   -0.11%     
============================================
  Files           113      113              
  Lines         25724    25216     -508     
  Branches       2511     2504       -7     
============================================
- Hits          24632    24120     -512     
- Misses          773      775       +2     
- Partials        319      321       +2
Flag Coverage Δ
#linux 95.46% <100%> (-0.12%) ⬇️
#osx 93.31% <100%> (+0.01%) ⬆️
#windows 93.58% <100%> (ø) ⬆️
Impacted Files Coverage Δ
src/_pytest/assertion/rewrite.py 95.47% <100%> (-0.02%) ⬇️
testing/test_modimport.py 81.81% <0%> (-1.52%) ⬇️
testing/code/test_source.py 95.53% <0%> (-1.07%) ⬇️
testing/test_argcomplete.py 68.11% <0%> (-0.9%) ⬇️
testing/test_compat.py 91.13% <0%> (-0.63%) ⬇️
testing/test_skipping.py 97.65% <0%> (-0.57%) ⬇️
src/_pytest/warnings.py 88.88% <0%> (-0.53%) ⬇️
src/_pytest/debugging.py 77.98% <0%> (-0.41%) ⬇️
src/_pytest/fixtures.py 97.91% <0%> (-0.31%) ⬇️
src/_pytest/logging.py 94.96% <0%> (-0.31%) ⬇️
... and 74 more

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 c7bbb2a...9cb71af. Read the comment docs.

@blueyed

blueyed commented Mar 1, 2019

Copy link
Copy Markdown
Contributor Author

Fails quite badly, e.g.

_______________________________ test_hello[2-5] ________________________________
arg1 = 2, arg2 = 5
    def test_hello(arg1, arg2):
>       assert 0, (arg1, arg2)
E       AttributeError: module '_pytest.assertion.rewrite' has no attribute '_format_explanation'
test_parametrize_functional2.py:5: AttributeError
=========================== 4 failed in 0.07 seconds ===========================

It is not clear to me how this works / is being called from - but at least it can be moved to the top it seems.

@blueyed blueyed changed the title Remove needless import in _pytest.assertion.rewrite Move import of _format_explanation in _pytest.assertion.rewrite Mar 1, 2019
@nicoddemus

nicoddemus commented Mar 1, 2019

Copy link
Copy Markdown
Member

Oh then it is probably being used by the rewritten AST.

@nicoddemus

nicoddemus commented Mar 1, 2019

Copy link
Copy Markdown
Member

And yes it can be moved to the top. 👍

@blueyed

blueyed commented Mar 1, 2019

Copy link
Copy Markdown
Contributor Author

Oh then it is probably being used by the rewritten AST.

Ah.. via https://github.com/blueyed/pytest/blob/9e9b140806b2a525ef0f830b627511b78830aea7/src/_pytest/assertion/rewrite.py#L857.

@blueyed blueyed merged commit 42561db into pytest-dev:features Mar 1, 2019
@blueyed blueyed deleted the remove-import branch March 1, 2019 14:13
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.

2 participants

@blueyed @nicoddemus