Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: expand versioned_branches feature to Python 3 minor version comparison (<, >, <=, >= with else) #530

Merged
merged 1 commit into from Sep 12, 2021

Conversation

neutrinoceros
Copy link
Contributor

Fix #488
It should cover Python 3.x branches (where x is >=0) using operators <, <=, >, >=, and assuming
that an else statement exists.

A couple remarks:

  • this is my very first contribution to anything that uses 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
  • I ended up kinda "hacking" existing functions (namely _fix_py3_block, _fix_py3_block_else and _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
  • I didn't cover the == 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 ?

@neutrinoceros
Copy link
Contributor Author

Also; should I add docs too ?

@neutrinoceros
Copy link
Contributor Author

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 neutrinoceros force-pushed the expand_versioned_branches branch 2 times, most recently from b044de8 to c85e410 Compare September 11, 2021 10:19
@neutrinoceros
Copy link
Contributor Author

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 neutrinoceros force-pushed the expand_versioned_branches branch 2 times, most recently from c59006e to 6999861 Compare September 11, 2021 10:41
@neutrinoceros neutrinoceros marked this pull request as ready for review September 11, 2021 10:49
Comment on lines 109 to 112
if state.settings.min_version >= (3,) and (
len(state.settings.min_version) >= 2
):
py3_minor = state.settings.min_version[1]
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

I think you can just set:

if state.settings.min_version == (3,):
    min_version = (3, 0)
else:
    min_version = state.settings.min_version

or something of the sort

@neutrinoceros neutrinoceros force-pushed the expand_versioned_branches branch 3 times, most recently from cf01e9f to c2696c2 Compare September 11, 2021 17:37
@neutrinoceros neutrinoceros marked this pull request as draft September 11, 2021 19:23
@neutrinoceros neutrinoceros force-pushed the expand_versioned_branches branch 5 times, most recently from e880ec6 to e0e1713 Compare September 12, 2021 08:35
@neutrinoceros
Copy link
Contributor Author

@asottile I was able to simplify the logic in _compare_to_3, I think it doesn't get much simpler from here. I also added some docs. Now ready for your next (final ?) review :)

@neutrinoceros neutrinoceros marked this pull request as ready for review September 12, 2021 08:55
@neutrinoceros
Copy link
Contributor Author

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.

Comment on lines +141 to +144
any(
_compare_to_3(node.test, (ast.Lt, ast.LtE), minor)
for minor in range(min_version[1])
)
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a plan :)

Copy link
Owner

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit 51e4532 into asottile:master Sep 12, 2021
@neutrinoceros neutrinoceros deleted the expand_versioned_branches branch September 13, 2021 05: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.

Feature idea/question: cleanup conditionals on sys.version_info
3 participants