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

functional: refactor common types utils into separate module #666

Conversation

tehrengruber
Copy link
Contributor

@tehrengruber tehrengruber commented Feb 13, 2022

The utilities in the FOAST type deduction are independent of the FOAST and only require the symbol types in ffront.common_types. This PR refactors them into a separate module so that I can use them in the PAST type deduction.

Note that the mypy version that we are currently using does not support pythons match statement used in the code. As such mypy fails and according to python/mypy#11829 there doesn't seem to be a way to suppress the warning. Support is expected to land in the next release 0.940 and is already on master: python/mypy#10191

@tehrengruber tehrengruber changed the title Functional: refactor common types utils into separate module functional: refactor common types utils into separate module Feb 13, 2022
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

Since these functions fully depend on the common_types module, I think it would make more sense to just add them there instead of creating a new separate module.

@tehrengruber
Copy link
Contributor Author

tehrengruber commented Feb 14, 2022

Alright, changed. I had placed them into separate models to only have types in common_types, but that's not a strict requirement. Shall we just ignore the mypy errors? They will eventually disappear anyway.

@egparedes
Copy link
Contributor

Shall we just ignore the mypy errors? They will eventually disappear anyway.

Maybe add the files to the ignore list in the pre-commit settings for mypy. When the next mypy release comes out in a couple of weeks, we can remove all files from the ignore list.

@egparedes
Copy link
Contributor

You need to rebase/merge the new functional branch here and it is ready to go.

@tehrengruber
Copy link
Contributor Author

I tried just to recognize that we apparently both missed that @DropD did almost the same change in #654. Well the utils landed in a new file type_info.py. We decided to put it in common_types to avoid a new module, but let's leave it that way for now. Otherwise merging my PAST PR becomes even more complex.

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.

2 participants