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

Future type annotations #165

Merged
merged 12 commits into from
Sep 15, 2022
Merged

Future type annotations #165

merged 12 commits into from
Sep 15, 2022

Conversation

janosh
Copy link
Member

@janosh janosh commented Sep 1, 2022

Future annotations are backwards compatible to py3.7 which is EOL Jun 2023.

@utf
Copy link
Member

utf commented Sep 6, 2022

Hi @janosh, thanks for this PR but it seems like it has broken the tests?

@janosh
Copy link
Member Author

janosh commented Sep 6, 2022

@utf Sorry, had forgotten about this. Looks like the failure arose from the distinction between annotations PEP 526 and type hints PEP 484.

Class variables in defect.py

wswqs: List[WSWQ] # noqa
dir_name: str = Field(
None, description="Directory where the WSWQ calculations are performed"
)
ref_dir: str = Field(
None, description="Directory where the reference W(0) wavefunction comes from"
)
distorted_dirs: List[str] = Field(
None,
description="List of directories where the distorted W(Q) wavefunctions come from",
)

are the former, i.e. annotations, not type hints. I'm surprised they don't appear to support future annotations below 3.10. Maybe I'm missing sth.

@janosh
Copy link
Member Author

janosh commented Sep 6, 2022

Ah, maybe it's actually an issue with pydantic. pydantic/pydantic#2760

Would explain why I've never seen such errors before. 😄

@utf
Copy link
Member

utf commented Sep 14, 2022

Still seems like there are issues? Should I close this for now?

@janosh
Copy link
Member Author

janosh commented Sep 14, 2022

@utf Hang on, I think this is easy to fix.

The exact cause of the problem is well described here: pydantic/pydantic#3300 (comment). The advice there is to update to 3.10 which is not an option here so instead I'll just revert to old type hints within children of pydantic.BaseModel.

config files supported since autoflake v1.5
    def _evaluate(self, globalns, localns):
        if not self.__forward_evaluated__ or localns is not globalns:
            if globalns is None and localns is None:
                globalns = localns = {}
            elif globalns is None:
                globalns = localns
            elif localns is None:
                localns = globalns
            self.__forward_value__ = _type_check(
>               eval(self.__forward_code__, globalns, localns),
                "Forward references must evaluate to types.",
                is_argument=self.__forward_is_argument__)
E           TypeError: 'type' object is not subscriptable
src/atomate2/vasp/schemas/{task,calculation}.py
src/atomate2/common/schemas/{molecule,elastic}.py
among others
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #165 (93d32dd) into main (e9f131d) will not change coverage.
The diff coverage is 86.66%.

@@           Coverage Diff           @@
##             main     #165   +/-   ##
=======================================
  Coverage   75.04%   75.04%           
=======================================
  Files          52       52           
  Lines        4685     4685           
  Branches      737      737           
=======================================
  Hits         3516     3516           
  Misses        983      983           
  Partials      186      186           
Impacted Files Coverage Δ
src/atomate2/amset/schemas.py 0.00% <ø> (ø)
src/atomate2/common/files.py 67.32% <ø> (ø)
src/atomate2/common/schemas/cclib.py 74.32% <0.00%> (ø)
src/atomate2/vasp/flows/phonons.py 94.38% <ø> (-0.07%) ⬇️
src/atomate2/vasp/jobs/amset.py 0.00% <0.00%> (ø)
src/atomate2/vasp/schemas/calculation.py 85.76% <ø> (ø)
src/atomate2/common/jobs.py 100.00% <100.00%> (ø)
src/atomate2/utils/datetime.py 100.00% <100.00%> (ø)
src/atomate2/utils/log.py 100.00% <100.00%> (ø)
src/atomate2/utils/path.py 93.33% <100.00%> (-0.22%) ⬇️
... and 8 more

@utf
Copy link
Member

utf commented Sep 15, 2022

Thanks! Would you also mind removing the new type hints in src/atomate2/amset/schemas.py and then I can merge

@janosh
Copy link
Member Author

janosh commented Sep 15, 2022

@utf Sure thing.

@utf
Copy link
Member

utf commented Sep 15, 2022

Great. Thank you!

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.

None yet

2 participants