8000 Future annotations by janosh · Pull Request #2452 · materialsproject/pymatgen · GitHub
[go: up one dir, main page]
Skip to content

Future annotations #2452

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 55 commits into from
Mar 11, 2022
Merged

Future annotations #2452

merged 55 commits into from
Mar 11, 2022

Conversation

janosh
Copy link
Member
@janosh janosh commented Mar 5, 2022

Convert annotations to future syntax available through import in Python 3.7 upwards. See PEP 563.

janosh added 21 commits March 5, 2022 10:26
@janosh
convert annotations to future syntax available from py37
2e480a0
@janosh
pymatgen/analysis/chempot_diagram.py
0358cd2
@janosh
pymatgen/analysis/interface_reactions.py
cc78075
@janosh
pymatgen/analysis/local_env.py
374bc46
@janosh
pymatgen/analysis/pourbaix_diagram.py
3929535
@janosh
pymatgen/analysis/diffraction/tem.py
c691ea8
@janosh
pymatgen/analysis/interfaces/coherent_interfaces.py
85006be
@janosh
pymatgen/analysis/interfaces/substrate_analyzer.py
ad0fc2e
@janosh
pymatgen/analysis/interfaces/zsl.py
bca8d68
@janosh
pymatgen/analysis/magnetism/analyzer.py
2f505f3
@janosh
pymatgen/analysis/magnetism/jahnteller.py
289394a
@janosh
pymatgen/analysis/xas/spectrum.py
ee40982
@janosh
pymatgen/apps/battery/battery_abc.py
5fe5219
@janosh
pymatgen/apps/battery/conversion_battery.py
ad5614a
@janosh
pymatgen/apps/battery/insertion_battery.py
64d77c0
@janosh
pymatgen/cli/pmg_config.py
7a5e9d0
@janosh
pymatgen/command_line/mcsqs_caller.py
dd43352
@janosh
pymatgen/core/composition.py
7ed3307
@janosh
pymatgen/core/interface.py
83c9874
@janosh
pymatgen/core/ion.py
aaf5137
@janosh
pymatgen/core/lattice.py
10c8c31
@coveralls
Copy link
coveralls commented Mar 5, 2022 •
edited
Loading

Coverage Status

Coverage decreased (-0.9%) to 83.24% when pulling 4d44b48 on janosh:future-annotations into eaa2b62 on materialsproject:master.

janosh added 19 commits March 5, 2022 12:25
@janosh
Copy link
Member Author
janosh commented Mar 7, 2022

Note to future pymatgen maintainers: pymatgen makes heavy use of variadic args (**kwargs in particular, 438 hits occurrences across the code base, and another 57 *args) which unfortunately can't yet be annotated. There's a mypy issue python/mypy#4441 from 2018 about this limitation still unclosed. Though it looks like the recently accepted PEP 646 slated for py3.11 with its Unpack type might make it easier to allow using TypedDicts to annotate **kwargs (at least pyright seems keen to try it: microsoft/pyright#3002). IMO definitely sth to explore once that becomes possible.

@mkhorton
Copy link
Member
mkhorton commented Mar 9, 2022

Hi @janosh, thanks for the PR -- can you point me to where in the PEP the x | None syntax is preferred over Optional[None]?

I was reading elsewhere that the new preferred option for kwargs may be to omit the Optional entirely, e.g. just your_kwarg: x = None, and that mypy will respect this.

@janosh
Copy link
Member Author
janosh commented Mar 9, 2022

I was reading elsewhere that the new preferred option for kwargs may be to omit the Optional entirely, e.g. just your_kwarg: x = None, and that mypy will respect this.

@mkhorton That's right! As I was discussing with @arosen93 over in #2451 (comment), kwarg: int | None = None and kwarg: int = None are equivalent unless you set mypy's --no-implicit-optional flag.

@janosh
Copy link
Member Author
janosh commented Mar 9, 2022

@mkhorton Regarding your 1st question, I believe 604 is the PEP you're looking for.

We will also be able to write t | None or None | t instead of Optional[t]:

isinstance(None, int | None)
isinstance(42, None | int)

@mkhorton
Copy link
Member
mkhorton commented Mar 9, 2022

Thanks, so just to summarize:

  • Optional[x] and x | None are equally accepted, with neither specifically preferred (presumably the latter is easier since it skips the import)
  • The implicit optionals, though accepted by mypy by default, are not currently "officially" preferred (i.e., no guidance from Python/PEPs on this)

Happy to merge this regardless since it looks cleaner, I just want to make sure we have guidelines for future PRs.

@janosh
Copy link
Member Author
janosh commented Mar 9, 2022 •
edited
Loading

Yes, whether you allow implicit optionals is up to personal preference afaik. Seems more pythonic to me so I would leave it enabled.

I just want to make sure we have guidelines for future PRs.

We already do actually. This will all be handled automatically by pre-commit.ci in the future since we're using the pyupgrade hook. It will auto rewrite all type hints to use the new leaner syntax in files that have the __future__'s import.

The only thing left to decide really is whether that from __future__ import annotations should be added to all files automatically.

@mkhorton mkhorton merged commit b20e8d6 into materialsproject:master Mar 11, 2022
@janosh janosh deleted the future-annotations branch March 11, 2022 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@janosh @coveralls @mkhorton
0