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

Enable strict typing #984

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Enable strict typing #984

merged 4 commits into from
Oct 25, 2023

Conversation

blink1073
Copy link
Contributor

Leave a few rules on for now that would need a lot more work:

  • warn_return_any = false
  • disallow_any_generics = false
  • no_implicit_reexport = false

# Define custom type for kernel connection info
KernelConnectionInfo = Dict[str, Union[int, str, bytes]]


def write_connection_file(
fname: Optional[str] = None,
fname: str | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

I dont' think we should do this if we still support 3.8 and 3.9, this beciome valid syntax, but typing.get_type_hints() will fail on | for Union syntax on 3.8 and 3.9, so we risk breaking things.

Copy link
Member

@Carreau Carreau Oct 23, 2023

Choose a reason for hiding this comment

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

from __future__ import annotations
from typing import Optional, get_type_hints


def write_connection_file(fname: str | None = None) -> None:
    pass


get_type_hints(write_connection_file)

$ python f.py
Traceback (most recent call last):
  File "/Users/bussonniermatthias/dev/jupyter_client/f.py", line 9, in <module>
    get_type_hints(write_connection_file)
  File "/Users/bussonniermatthias/miniforge3/envs/39/lib/python3.9/typing.py", line 1386, in get_type_hints
    value = _eval_type(value, globalns, localns)
  File "/Users/bussonniermatthias/miniforge3/envs/39/lib/python3.9/typing.py", line 254, in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
  File "/Users/bussonniermatthias/miniforge3/envs/39/lib/python3.9/typing.py", line 493, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof, that is frustrating. I've already released several changes with that syntax in other jupyter libs. Thanks for catching that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not your fault, it's a common confusion. I wouldn't have caught it if not for a similar problem recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well now I'm not so sure what to do, because this also applies to type[], set[], list[], dict[], etc. I don't think we need to claim to support type hints for Python 3.8 or 3.9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least not at run time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing prevents folks from calling get_type_hints on their own code, I just verified.

Copy link
Member

Choose a reason for hiding this comment

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

as you made the PR i think it's fine, but maybe we ca avoid this for subsequent libs.

Copy link
Contributor Author

@blink1073 blink1073 Oct 24, 2023

Choose a reason for hiding this comment

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

Yeah that sounds like a good plan. I added this to ignores, which I'll use as a template:

  # non-pep585-annotation
  "UP006",
  # non-pep604-annotation
  "UP007",

@blink1073 blink1073 enabled auto-merge (squash) October 24, 2023 23:53
@blink1073 blink1073 merged commit ff94e31 into jupyter:main Oct 25, 2023
29 checks passed
@blink1073 blink1073 deleted the ruff-ann branch October 25, 2023 00:04
@Carreau
Copy link
Member

Carreau commented Oct 25, 2023

BTW @blink1073 when I run mypy directly instead of via hatch, I get many more errors. Any idea why ?

@blink1073
Copy link
Contributor Author

The hatch version has some min version pins to pick up newer typings.

@Carreau
Copy link
Member

Carreau commented Oct 27, 2023

The hatch version has some min version pins to pick up newer typings.

Yeah it helps to update the versions, but I still get some errors.

That's fine I can use hatch for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants