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

Implement most common sys.version_info and sys.platform checks #1942

Merged
merged 9 commits into from
Jul 27, 2016

Conversation

gvanrossum
Copy link
Member

Addresses most but not all of #698. (The first two bullets of my plan.)

def is_sys_attr(expr: Node, name: str) -> bool:
if isinstance(expr, MemberExpr) and expr.name == name:
if isinstance(expr.expr, NameExpr) and expr.expr.name == 'sys':
# TODO: Guard against a local named sys, etc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also this won't work with things like import sys as _sys -- I found 8 files that did this in the Python 2.7 stdlib.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another thing that doesn't work is from sys import platform (which is arguably questionable style and doesn't seem very common). Not sure if we should support it -- one option is to describe it as unsupported in our docs.

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 27, 2016

I did a very quick pass and looks reasonable -- I won't have time to review this in detail since I'm going to be on vacation, so please don't wait until you get more feedback :-)

A few more ideas for test cases (though at least some of these are probably covered by some existing tests):

  • A test that has a check within a function or class body instead of module top level.
  • A test that conditionally defines a class.
  • A test that conditionally imports a module.
  • A test that conditionally defines a variable.

@gvanrossum
Copy link
Member Author

gvanrossum commented Jul 27, 2016 via email

@gvanrossum gvanrossum force-pushed the sys-version-info branch 2 times, most recently from 2ae5ef1 to 65cf9ac Compare July 27, 2016 17:57
@gvanrossum gvanrossum merged commit 0b82463 into master Jul 27, 2016
@gvanrossum gvanrossum deleted the sys-version-info branch July 27, 2016 18:32
@eric-wieser
Copy link
Contributor

Supporting sys.version_info.major would be a nice addition

@gvanrossum
Copy link
Member Author

gvanrossum commented Mar 2, 2018

I opened #4661 for this -- but note that we'd need buy-in from PyCharm and Google's pytype as well before we can start using this in stubs (where it's most useful).

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.

3 participants