CLN: Use pandas.compat instead of sys.version_info for Python version checks #20545

jschendel · 2018-03-30T02:41:28Z

Hopefully this will make code changes related to dropping support for specific versions of Python slightly easier to identify.

Summary:

Replaced instances sys.version_info with equivalent checks from pandas.compat
Removed code blocks specific to unsupported versions of Python (<2.7, 3.0-3.4)

There were some very specific examples that I left as-is:

pandas/pandas/tests/io/formats/test_to_csv.py

Line 13 in c4b4a81

@pytest.mark.xfail((3, 6, 5) > sys.version_info >= (3, 5),

Didn't change any Cython related code, as it appears that we want to avoid imports from outside _libs:

pandas/pandas/_libs/tslibs/parsing.pyx

Lines 25 to 26 in c4b4a81

    
           # Avoid import from outside _libs 
        
           if sys.version_info.major == 2:

jschendel · 2018-03-30T03:12:18Z

pandas/util/testing.py

@@ -2088,7 +2088,7 @@ def dec(f):
 # and conditionally raise on these exception types
 _network_error_classes = (IOError, httplib.HTTPException)

-if sys.version_info >= (3, 3):
+if PY3:


Since 3.0 - 3.4 are no longer supported, seems fine to use PY3. Could use PY35 if being explicit would be preferred here (or maybe just leave as-is).

I'm fine with this change. We should be encouraging code that relies solely on the supported versions of Python and not do the C-esque thing of making it super-backwards-compatible 😄

jschendel · 2018-03-30T03:14:51Z

pandas/tests/scalar/timestamp/test_comparisons.py

@@ -104,7 +103,7 @@ def test_cant_compare_tz_naive_w_aware(self):
        pytest.raises(Exception, b.__lt__, a)
        pytest.raises(Exception, b.__gt__, a)

-        if sys.version_info < (3, 3):
+        if PY2:


Likewise, <3.3 implies PY2 based on the currently supported versions. Could be more explicit if desired though.

That's reasonable. 2.7, 3.5 - 3.6 are the only supported versions.

codecov · 2018-03-30T05:17:43Z

Codecov Report

Merging #20545 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #20545      +/-   ##
==========================================
- Coverage   91.84%   91.81%   -0.03%     
==========================================
  Files         152      152              
  Lines       49268    49266       -2     
==========================================
- Hits        45250    45236      -14     
- Misses       4018     4030      +12

Flag	Coverage Δ
#multiple	`90.2% <100%> (-0.03%)`	⬇️
#single	`41.89% <100%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/formats/terminal.py	`20.98% <100%> (ø)`	⬆️
pandas/util/testing.py	`84.73% <100%> (ø)`	⬆️
pandas/compat/__init__.py	`57.74% <100%> (ø)`	⬆️
pandas/io/clipboard/clipboards.py	`30.58% <100%> (-1.6%)`	⬇️
pandas/plotting/_converter.py	`65.07% <0%> (-1.74%)`	⬇️

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 d7a4f5b...6cf2e98. Read the comment docs.

jorisvandenbossche

Looks good to me.
Thanks for the clean-up!

jreback · 2018-03-30T17:39:10Z

lgtm. the only thing would add is if you can fail lint.sh on a grep for sys.version_info would be great (might need to exclude the .pyx for now on that though)

jschendel · 2018-03-30T19:48:31Z

I can add a check for this to lint.sh. There are a situations where sys.version_info is still valid though, as there are some very specific checks that probably don't make sense to encode in pandas.compat, eg:

pandas/pandas/tests/io/formats/test_to_csv.py

Line 13 in c4b4a81

@pytest.mark.xfail((3, 6, 5) > sys.version_info >= (3, 5),

This would get caught by lint.sh, but I'm guessing we'd rather be more aggressive here, and selectively allow failing cases to be merged when appropriate? I'm assuming (possibly mistakenly) that lint.sh only runs over newly added code? If it hits the entire codebase, we might not want to do this since the corner cases will fail every time. I suppose I could try checking for specific examples that are clearly invalid if this is the case, though it seems like that could get out of sync as support for versions are added/dropped.

jreback · 2018-03-30T20:01:11Z

we have a PY35 already. guess could just exclude this file) The lint check just enforces that the code all uses the same pattern, don't like to have people making up ways to do very standard things (and docs on how to do things like this, while helpful, get out of date), better to have an actual check.

no lint.sh runs over ALL code. as i said, you might need to exclude .pyx files as we (can't) import pandas.compat there, but otherwise I see no reason not to enforce this (and if there is a specific case pls point it out).

jschendel · 2018-03-30T21:02:50Z

The example I showed is actually different than PY35. It's only skipping for 3.5-3.6.4; the test should pass for 2.7 and 3.6.5+. Something like PY2 or PY365 would work, so it'd be a matter of adding PY365.

That actually looks like the only oddly specific case of a version check. The only other non-pandas.compat instance of sys.version_info that I can't get rid of is in util/_print_versions.py:

pandas/pandas/util/_print_versions.py

Line 41 in 0a00365

("python", '.'.join(map(str, sys.version_info))),

Could either skip that file or create a variable like PYVERSION = sys.version_info in pandas.compat and import; seems like PYVERSION could just be abused like sys.version_info though, so will go with the skip route unless told otherwise.

gfyoung

Nice fixes!

@jreback

jreback · 2018-03-30T21:15:31Z

yeah have special cases is a bit annoying. how about you just remove the remaining ones that are possible? and leave lint.sh alone is ok.

jschendel · 2018-03-30T21:55:55Z

Added some small fixes for the remaining easily removable instances of sys.version_info.

jreback · 2018-03-30T22:04:40Z

great. can you rebase. just merged a patch which conflicts.

jreback · 2018-03-30T22:05:52Z

pandas/tests/io/parser/common.py

@@ -1277,8 +1277,7 @@ def test_verbose_import(self):

    def test_iteration_open_handle(self):


can you change this to a pytest.mark.skip decorator

… checks

jschendel · 2018-03-30T22:17:22Z

rebased and made the requested change

jreback · 2018-03-31T16:01:21Z

thanks!

… checks (pandas-dev#20545)

jschendel commented Mar 30, 2018

View reviewed changes

jorisvandenbossche reviewed Mar 30, 2018

View reviewed changes

jreback added Testing pandas testing functions or related to the test suite 2/3 Compat Compat pandas objects compatability with Numpy or Python functions labels Mar 30, 2018

jreback added this to the 0.23.0 milestone Mar 30, 2018

gfyoung approved these changes Mar 30, 2018

View reviewed changes

jreback reviewed Mar 30, 2018

View reviewed changes

jschendel added 3 commits March 30, 2018 16:15

CLN: Use pandas.compat instead of sys.version_info for Python version…

06cd377

… checks

fix additional cases hit by grep

37010ff

mark skipif

6cf2e98

jschendel force-pushed the compat-sys-version branch from cfa5a45 to 6cf2e98 Compare March 30, 2018 22:16

jreback merged commit 14f03ce into pandas-dev:master Mar 31, 2018

kornilova203 pushed a commit to kornilova203/pandas that referenced this pull request Apr 23, 2018

CLN: Use pandas.compat instead of sys.version_info for Python version…

c1aaaa2

… checks (pandas-dev#20545)

jschendel deleted the compat-sys-version branch September 24, 2018 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: Use pandas.compat instead of sys.version_info for Python version checks #20545

CLN: Use pandas.compat instead of sys.version_info for Python version checks #20545

jschendel commented Mar 30, 2018

jschendel Mar 30, 2018

gfyoung Mar 30, 2018 •

edited

Loading

jschendel Mar 30, 2018

gfyoung Mar 30, 2018 •

edited

Loading

codecov bot commented Mar 30, 2018 •

edited

Loading

jorisvandenbossche left a comment

jreback commented Mar 30, 2018 •

edited

Loading

jschendel commented Mar 30, 2018

jreback commented Mar 30, 2018 •

edited

Loading

jschendel commented Mar 30, 2018 •

edited

Loading

gfyoung left a comment

jreback commented Mar 30, 2018

jschendel commented Mar 30, 2018

jreback commented Mar 30, 2018

jreback Mar 30, 2018

jschendel commented Mar 30, 2018

jreback commented Mar 31, 2018

	# Avoid import from outside _libs
	if sys.version_info.major == 2:

		@@ -1277,8 +1277,7 @@ def test_verbose_import(self):

		def test_iteration_open_handle(self):

CLN: Use pandas.compat instead of sys.version_info for Python version checks #20545

CLN: Use pandas.compat instead of sys.version_info for Python version checks #20545

Conversation

jschendel commented Mar 30, 2018

jschendel Mar 30, 2018

Choose a reason for hiding this comment

gfyoung Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

jschendel Mar 30, 2018

Choose a reason for hiding this comment

gfyoung Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 30, 2018 • edited Loading

Codecov Report

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jreback commented Mar 30, 2018 • edited Loading

jschendel commented Mar 30, 2018

jreback commented Mar 30, 2018 • edited Loading

jschendel commented Mar 30, 2018 • edited Loading

gfyoung left a comment

Choose a reason for hiding this comment

jreback commented Mar 30, 2018

jschendel commented Mar 30, 2018

jreback commented Mar 30, 2018

jreback Mar 30, 2018

Choose a reason for hiding this comment

jschendel commented Mar 30, 2018

jreback commented Mar 31, 2018

gfyoung Mar 30, 2018 •

edited

Loading

gfyoung Mar 30, 2018 •

edited

Loading

codecov bot commented Mar 30, 2018 •

edited

Loading

jreback commented Mar 30, 2018 •

edited

Loading

jreback commented Mar 30, 2018 •

edited

Loading

jschendel commented Mar 30, 2018 •

edited

Loading