-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-39999: Improve compatibility of the ast module. #19056
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
Conversation
* Re-add removed classes Suite, Param, AugLoad and AugStore. * Add docstrings for dummy classes. * Add docstrings for attribute aliases. * Set __module__ to "ast" instead of "_ast".
Lib/ast.py
Outdated
|
||
class Suite(mod): | ||
"""Unused AST node class.""" | ||
|
||
class AugLoad(expr_context): | ||
"""Unused AST node class.""" | ||
|
||
class AugStore(expr_context): | ||
"""Unused AST node class.""" | ||
|
||
class Param(expr_context): | ||
"""Unused AST node class""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we deprecate this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used word "deprecated" if it was used in past Python releases and the user code can use them for compatibility. I used word "unused" if it was not used in Python (at least in Python 3) and there is no reason to use it in the user code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But aren't them will be removed in time or stay them forever? If they will, wouldn't it be good to start doing warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to start emitting warnings when the user code can just drop these classes. Currently Param is needed for 2.7, and some projects still support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently Param is needed for 2.7, and some projects still support it.
Oh, that makes sense.
Lib/ast.py
Outdated
Constant.n = property(_getter, _setter) | ||
Constant.s = property(_getter, _setter) | ||
Constant.n = property(_getter, _setter, doc='Deprecated. Use value instead.') | ||
Constant.s = property(_getter, _setter, doc='Deprecated. Use value instead.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to emit a DeprecationWarning when it's used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is possible. But it would be better to start emitting warnings when the user code no longer need to use the deprecated feature. I.e. when the support of Python 3.7 be dropped in most projects which use AST.
@@ -564,22 +568,40 @@ def __new__(cls, *args, **kwargs): | |||
type(...): 'Ellipsis', | |||
} | |||
|
|||
class Index(AST): | |||
class slice(AST): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@serhiy-storchaka what do you think about implementing custom instance and subclass checks for slice
? slice
is a sum type, so there should be checks like this issubclass(thing, ast.slice)
or isinstance(thing, ast.slice)
something like this should work
def __subclasscheck__(self, subclass):
if issubclass(subclass, Slice):
return True
else:
return super().__subclasscheck__(subclass)
def __instancecheck__(self, instance):
if isinstance(instance, Slice):
return True
else:
return super().__instancecheck__(instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is Slice
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the simplification, it was part of slice
kind. Now it is an expression by it is own but if we want to increase compatibility, wouldn't it better to it behave like a subclass of ast.slice
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it would be useful because Index()
can return arbitrary node type and it is impossible to distinguish ExtSlice
from Tuple
.
* Re-add removed classes Suite, slice, Param, AugLoad and AugStore. * Add docstrings for dummy classes. * Add docstrings for attribute aliases. * Set __module__ to "ast" instead of "_ast".
__module__
to "ast" instead of "_ast".https://bugs.python.org/issue39999