Use packaging/pkg_resources to check minversion #4379
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4379 +/- ##
=========================================
+ Coverage 95.88% 96.48% +0.6%
=========================================
Files 111 111
Lines 25008 28297 +3289
Branches 2438 2917 +479
=========================================
+ Hits 23979 27303 +3324
+ Misses 725 706 -19
+ Partials 304 288 -16
Continue to review full report at Codecov.
|
|
Note that "packaging" was used for pytest-sugar: https://github.com/Frozenball/pytest-sugar/pull/160/files#diff-5205b06d924f1150750e0cc9f8326313R17 |
|
what's the rationale for switching at all -- |
|
Just for reference: Teemu/pytest-sugar#160 (comment) |
|
Packaging is preferable here. pkg_resources only wraps packaging (and incidentally has to vendor it).
Also, distutils is deprecated. I'd recommend not importing it at all if possible. |
|
I understand that |
|
I tried to change it to use it fails with (note, I used py37 because I'm using python 3.7 in my dev environment) |
Oh I was under the impression we already depended on If that's not the case I agree with @asottile's view that it doesn't seem worthwhile just because a very small bit of functionality like parse version, but on the other hand I'm not a PyPA expert like @jaraco and @RonnyPfannschmidt, so I will defer that decision to them if they know it is the right one. |
|
@hoylemd when changing dependencies you'll probably need -r (recreate) when invoking tox |
|
@asottile thanks! that got it. Pushed up a commit using |
There was a problem hiding this comment.
Sorry for all the bike-shedding.
I think we should leave the check as-is (i.e. do not apply anything from this PR).
From #4315 the only issue left would be to have a test for this.
I'll close this for now, please do not hesitate from commenting/re-opening.
I'd go further to say that use of distutils is generally discouraged if not yet formally deprecated, even modules and functionality that's not specifically related to packaging (like the shell/filesystem utilities). It will likely be years before distutils is actually deprecated, but it would be helpful if packages like pytest and efforts like the one proposed could be accepted as a gesture and example of best practices. I don't feel strongly about it. |
|
@jaraco As for pytest this is really only about getting its own version parsing more straight (it failed with 4.10 after all). We'll pretty sure adjust to any DeprecationWarning etc, but installing another package just for this feels overbloated currently to me.
Me neither. |
|
im just revisiting this pr as im taking a closer look and i am quite saddened by the history of this one - we should as soon as possible and as quickly as possible remove the need for as long as we require |
|
marked as wip to prevent accidents |
Use pkg_resources.parse_version in minver check Add meself to AUTHORS & changelog Format CHANGELOG
00c6242 to
1568e38
Compare
|
@blueyed Done! |
Checklist:
changelogfolder, with a name like<ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.masterbranch for bug fixes, documentation updates and trivial changes.featuresbranch for new features and removals/deprecations.AUTHORSin alphabetical order;Fix #4315
Tasks from The issue:
I couldn't figure out the best way to add an acceptance test, but I found that
testing/test_config.py::TestParseIni.test_tox_ini_wrong_versionwas already checking the behaviour. I tried to add a 'right version' test by copying that one and changing the minver in the generated ini to 3.0, but whenever I ran it I'd be getting errors about reading from stdin. I couldn't figure out what was causing the errors, so I just moved on.note: I tested this with python 3.7, not 3.6 - it was pretty easy to get that to work. Easier than setting 3.6 up in my dev environment at least. I assume CI will do 3.6, so we'll still have all the coverage and I don't anticipate any issues cropping up.