ENH: expand versioned_branches feature to Python 3 minor version comparison (<, >, <=, >= with else)#530
Conversation
|
Also; should I add docs too ? |
|
Ah, I see that this actually only gets 3/4 cases right from the associated issue's example, so this is definitely not complete, but right now it's bedtime for me, I'll add and fix more tests tomorrow ! |
neutrinoceros
force-pushed
the
expand_versioned_branches
branch
2 times, most recently
from
b044de8 to
c85e410
Compare
|
Alright, after a couple iterations (more tests and bugfixes), this is now in a state that covers every case I can currently think of correctly 🎉 |
neutrinoceros
force-pushed
the
expand_versioned_branches
branch
2 times, most recently
from
c59006e to
6999861
Compare
neutrinoceros
force-pushed
the
expand_versioned_branches
branch
from
6999861 to
9cca9f7
Compare
| if state.settings.min_version >= (3,) and ( | ||
| len(state.settings.min_version) >= 2 | ||
| ): | ||
| py3_minor = state.settings.min_version[1] |
There was a problem hiding this comment.
this makes a sometimes-present variable which is a bit of a hazard
the whole patch can be very much simplified by changing (3,) => (3, 0) and then you don't need any special logic
There was a problem hiding this comment.
this makes a sometimes-present variable which is a bit of a hazard
point taken.
the whole patch can be very much simplified by changing (3,) => (3, 0) and then you don't need any special logic
Not so sure about that: state.settings.min_version isn't guaranteed to have a second item (with --py3-plus it's just (3,)), but (3,) >= (3, 0) evaluates to False. Should I change this ?
More generally I think you're right that it can be simplified, at least it feels a lot like it, I just don't see how right now.
There was a problem hiding this comment.
I think you can just set:
if state.settings.min_version == (3,):
min_version = (3, 0)
else:
min_version = state.settings.min_versionor something of the sort
neutrinoceros
force-pushed
the
expand_versioned_branches
branch
3 times, most recently
from
cf01e9f to
c2696c2
Compare
asottile
force-pushed
the
expand_versioned_branches
branch
from
c2696c2 to
498fb74
Compare
neutrinoceros
force-pushed
the
expand_versioned_branches
branch
5 times, most recently
from
e880ec6 to
e0e1713
Compare
|
@asottile I was able to simplify the logic in |
neutrinoceros
force-pushed
the
expand_versioned_branches
branch
from
0b238b9 to
c0c37a0
Compare
|
added a "simplification" that avoids repeating a check when the minor version is > 0, but I'm not sure it's worth the obfuscation. Leaving it as an isolated commit so it can be easily pruned or squashed. |
| any( | ||
| _compare_to_3(node.test, (ast.Lt, ast.LtE), minor) | ||
| for minor in range(min_version[1]) | ||
| ) |
There was a problem hiding this comment.
I think this any block could be eliminated and subsumed into the _compare_to function
right now it's linear on the version being compared which could potentially be bad (?)
I'm going to merge this as is and we can always fix it later?
There was a problem hiding this comment.
Sounds like a plan :)
…arison (<, >, <=, >= with else)
asottile
force-pushed
the
expand_versioned_branches
branch
from
4753f82 to
31546d2
Compare
3 participants
Fix #488
It should cover Python 3.x branches (where x is >=0) using operators
<,<=,>,>=, and assumingthat an
elsestatement exists.A couple remarks:
ast, so I'm kind of astonished that my own tests are passing, but I'll gladly take any suggestions on how to make this better_fix_py3_block,_fix_py3_block_elseand_fix_py2_block) in the sense that I'm using them without modifications in a different fashion compared to their original intended usage. I suppose they ought to be renamed to better reflect what they actually do here (fix and if/else block and keep the top or lower half ?), but I leave this as a question to the maintainer==operator. I can do it if you feel it's necessary, but I thought I'd ask for feedback first.So... what do you think ? anything obvious that I'm not covering in the tests yet ?