-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add typing.Final and simplify type inference #1683
Conversation
Hello again! Thank you for this new pull request 🤩. Please begin by requesting your checklist using the command |
…ents for annotations
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.
Features I haven't used yet have helped me gain more insights for the project.
Hey @yguclu, @EmilyBourne, this PR is looking pretty good. @EmilyBourne and @Farouk-Echaref think it is ready to merge. Could you add your expertise to confirm that this follows all the coding conventions and fits in Pyccel's future plans? Thanks 😄 |
/bot run pr_tests |
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.
There seems to be lines in this PR which aren't tested. Please take a look at my comments and add tests which cover the new code.
If this is modified code which cannot be easily tested in this PR please open an issue to request that this code be either removed or tested. Once you have done that please leave a message on the relevant conversation beginning with the line /bot accept
and referencing the issue.
Similarly if the new code cannot be tested for some reason, please leave a comment beginning with the line /bot accept
on the relevant conversation explaining why the code can't be tested.
|
||
@is_const.setter | ||
def is_const(self, val): | ||
if not isinstance(val, bool): |
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.
This code isn't tested. Please can you take a look
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.
/bot accept
This doesn't really make sense at the moment but it is necessary to describe everything that is possible according to our type grammar.
Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with |
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.
Good job ! Your PR is using all the code it added/changed.
Add support for
typing.Final
. Fixes #1680 . This means that the AST can describe everything that textx parses in type string annotations. Therefore the type annotation methods are simplified and unified to reduce code duplication. Fixes #1581. This should make it simpler to add support for tuple/list/set type annotationsCommit Summary
Variable
andAnnotatedPyccelSymbol
(lambda expressions can't be pickled) by movingast.core.apply
->ast.core.apply_pickle
UnionType
SyntacticTypeAnnotation
to only handle one type (instead of all textx output).typing.Final
(representsis_const
concept)python_builtin_datatypes
and simply use_visit_PyccelSymbol
to collect type.It is not useful to differentiate between datatypes and functions. The two dictionaries point at the same objects and Python itself doesn't differentiate between the two. Anything that was missing was moved into buitin_functions.
_get_indexed_type
function to handleIndexedElement
objects which describe a type.float
s were passed instead of NumPy floats.is_const
setter to type annotations.