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

Prevent list method from shadowing list in type annotations #184

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sterliakov
Copy link

@sterliakov sterliakov commented Jan 23, 2025

Currently some features do not support type checking with mypy due to invalid type hints in the library. For example, the following:

issues = gh.rest.issues.list_for_repo("python", "mypy", state="open", sort="created").parsed_data
for i in issues: ...

will produce mypy errors. A small repro to explain the problem:

class Foo:
    def list(self) -> None: ...
    
    def method(self) -> list[str]:  # E: Function "__main__.Foo.list" is not valid as a type  [valid-type]
        return [""]

Foo().method().append("")  # E: "list?[builtins.str]" has no attribute "append"  [attr-defined]

The most common solution for this problem is to use builtins.list in such cases, which I do in this PR. Unfortunately, the schema was recently updated, so there are unrelated diffs withs moved/added classes.

To prevent similar issues from appearing in future, I suggest to add a mypy (and probably pyright) check to the project CI. I will submit a PR to do so once this is merged (those changes are smaller but still overlap with codegen, because we need a few ignore comments until https://peps.python.org/pep-0747/ is approved and implemented). There are a few type errors to correct, but those are mostly simple. In particular, #176 introduced a bug: QueryParamTypes and HeaderTypes must stick to dict and list because upstream httpx declares so and explicitly checks with isinstance, so passing some other Mapping or Sequence will either result in a crash or produce unexpected results.

@yanyongyu
Copy link
Owner

yanyongyu commented Jan 24, 2025

For the builtin type shallowing problem, I need to do some check about this. This PR includes too much changes and i can not review on it. Could you please commit the necessary code only without re-generate the openapi code?

I suggest to add a mypy (and probably pyright) check to the project CI

Currently, githubkit is checked with pyright, excluding the generated code. I will add an pyright action first to check if there are other issues.

Mypy is not primary supported, it will produce many type inferencing errors in complex cases (such as githubkit's paginator).

QueryParamTypes and HeaderTypes must stick to dict and list because upstream httpx declares so and explicitly checks with isinstance

httpx defines the types here. I'm not sure which version of httpx you are using?

@yanyongyu yanyongyu added the bug Something isn't working label Jan 24, 2025
@sterliakov
Copy link
Author

Yes, sure! I'll update this tomorrow to keep (the only) non-autogen change in this PR.

Re mypy: actually after fixing autogen there were only ~10 non-strict mypy errors, all either trivial to fix or worth a type-ignore. I can push and show it after merging solution to this critical issue.

Re httpx: sorry, my bad, HeaderTypes are fine, but QueryParamTypes were changed incorrectly. httpx expects

QueryParamTypes = Union[
    "QueryParams",
    Mapping[str, Union[PrimitiveData, Sequence[PrimitiveData]]],
    List[Tuple[str, PrimitiveData]],
    Tuple[Tuple[str, PrimitiveData], ...],
    str,
    bytes,
]

either a list or a tuple of Tuple[str, PrimitiveData]. It does not allow other sequences, and here's why:

https://github.com/encode/httpx/blob/10b7295922741b91a15751029e6ad3e8e5efb9f3/httpx/_urls.py#L438

Replacing that with Sequence[Tuple[str, PrimitiveData]] is not compatible with upstream httpx.

@sterliakov
Copy link
Author

I pushed the isolated change; all other files are modified by autogen - please run it yourself in whichever way is convenient for you.

@yanyongyu
Copy link
Owner

I checked the example code in pyright, the shadow issue does not happen. I'm not sure why the behavior is different between pyright and mypy.

If i remove the from __future__ import annotations, pyright also complains about the shadow error. I think this may related to the postponed eval of types.

@sterliakov
Copy link
Author

Hm, thanks, that might indeed be a mypy issue. I filed python/mypy#18525 and convert this to draft for now.

@sterliakov sterliakov marked this pull request as draft January 24, 2025 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants