Skip to content

Remove some dead code#4615

Merged
asottile merged 1 commit into
pytest-dev:featuresfrom
asottile:some_dead_code
Jan 14, 2019
Merged

Remove some dead code#4615
asottile merged 1 commit into
pytest-dev:featuresfrom
asottile:some_dead_code

Conversation

@asottile

@asottile asottile commented Jan 8, 2019

Copy link
Copy Markdown
Member
  • I wrote a thing: https://github.com/asottile/dead
  • wanted to try it out, there's lots of false positives and I didn't look
    through all the things it pointed out but here's some

Comment thread src/_pytest/junitxml.py
@@ -27,10 +28,6 @@
# Python 2.X and 3.X compatibility
if sys.version_info[0] < 3:
from codecs import open

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.

i believe it should be possible to go for io.open by now

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah I thought about this, but decided to leave it for another PR given codecs.open works more like python2.x open:

>>> from codecs import open
>>> with open('f', 'w', encoding='utf-8') as f:
...     f.write(b'wat')
... 
>>> import io
>>> with io.open('f', 'w', encoding='utf-8') as f:
...     f.write(b'wat')
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
TypeError: write() argument 1 must be unicode, not str

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.

most interesting, but also something we should unify towards the unicode only behaviour in junitxml

Comment thread src/_pytest/fixtures.py
Comment thread src/_pytest/assertion/rewrite.py
source.lines[:] = self.lines[start:end]
return source

def putaround(self, before="", after="", indent=" " * 4):

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.

those methods are potentially breaking accidential external api, we should document that as removal

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍 definitely, I'll make sure to list these out tomorrow in a changelog entry -- already targeting features

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe give a depreciation grace period so we don't have as many breaks as the recent mark changes?

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.

I agree, IMO we should avoid further breakages. 👍 for adding a deprecation warning.


def test_getrange(self):
x = self.source[0:2]
assert x.isparseable()

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.

ouch

Comment thread testing/python/metafunc.py
@codecov

codecov Bot commented Jan 8, 2019

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #4615      +/-   ##
============================================
+ Coverage     95.76%   95.76%   +<.01%     
============================================
  Files           111      111              
  Lines         24683    24566     -117     
  Branches       2446     2436      -10     
============================================
- Hits          23637    23526     -111     
+ Misses          739      735       -4     
+ Partials        307      305       -2
Flag Coverage Δ
#docs 29.62% <42.85%> (+0.13%) ⬆️
#doctesting 29.62% <42.85%> (+0.13%) ⬆️
#linting 29.62% <42.85%> (+0.13%) ⬆️
#linux 95.58% <100%> (-0.01%) ⬇️
#nobyte 92.39% <100%> (+0.01%) ⬆️
#numpy 93.2% <100%> (+0.01%) ⬆️
#pexpect 42.18% <42.85%> (+0.05%) ⬆️
#py27 93.8% <100%> (+0.01%) ⬆️
#py34 91.86% <100%> (+0.05%) ⬆️
#py35 91.88% <100%> (+0.05%) ⬆️
#py36 91.9% <100%> (+0.05%) ⬆️
#py37 93.92% <100%> (-0.01%) ⬇️
#trial 93.2% <100%> (+0.01%) ⬆️
#windows 93.94% <100%> (ø) ⬆️
#xdist 93.78% <100%> (-0.03%) ⬇️
Impacted Files Coverage Δ
src/_pytest/assertion/rewrite.py 95.75% <ø> (+0.14%) ⬆️
src/_pytest/terminal.py 91.83% <ø> (+0.07%) ⬆️
testing/test_session.py 96.37% <ø> (ø) ⬆️
src/_pytest/reports.py 97.4% <ø> (-0.22%) ⬇️
testing/code/test_source.py 96.38% <ø> (-0.16%) ⬇️
src/_pytest/fixtures.py 97.91% <ø> (-0.01%) ⬇️
src/_pytest/nodes.py 95.97% <ø> (+0.45%) ⬆️
src/_pytest/_code/source.py 91.87% <ø> (+1.13%) ⬆️
testing/python/metafunc.py 95.21% <ø> (ø) ⬆️
src/_pytest/python.py 93.09% <ø> (+0.09%) ⬆️
... and 9 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 5dcb370...16546b7. Read the comment docs.

Comment thread src/_pytest/nodes.py
extra_keywords.update(item.extra_keyword_matches)
return extra_keywords

def listnames(self):

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.

external api breach of node

@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.

Other than removing the potentially external APIs, great work! 😁

source.lines[:] = self.lines[start:end]
return source

def putaround(self, before="", after="", indent=" " * 4):

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.

I agree, IMO we should avoid further breakages. 👍 for adding a deprecation warning.

newsource.lines[:] = deindent(self.lines)
return newsource

def isparseable(self, deindent=True):

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.

Ditto about deprecating it first.

Comment thread src/_pytest/junitxml.py
# Python 2.X and 3.X compatibility
if sys.version_info[0] < 3:
from codecs import open
else:

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.

Great!

- I wrote a thing: https://github.com/asottile/dead
- wanted to try it out, there's lots of false positives and I didn't look
  through all the things it pointed out but here's some
@rtd-helper

rtd-helper Bot commented Jan 14, 2019

Copy link
Copy Markdown

The rtd-bot is activated, but no .github/config.yml found in this repository.
Make sure that you have it in your default branch.

@asottile

Copy link
Copy Markdown
Member Author

been a bit busy, I put back the potentially public things and I can follow up with those later, don't really have time to document all the deprecations right now but wanted to get at least this bit out

@nicoddemus

Copy link
Copy Markdown
Member

Failures are unrelated:

E       DeprecationWarning: np.asscalar(a) is deprecated since NumPy v1.16, use a.item() instead

We should fix them on master anyway.

@nicoddemus

Copy link
Copy Markdown
Member

#4643 fixes the deprecation error.

@asottile asottile merged commit 5bb0be1 into pytest-dev:features Jan 14, 2019
@asottile asottile deleted the some_dead_code branch January 14, 2019 15:35
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.

4 participants