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

Add support for TypedDict #128

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

Conversation

danjones1618
Copy link

@danjones1618 danjones1618 commented Jun 1, 2024

[!INFO]
This PR is to related to resolving issue #127

TypedDicts are used only within the type system for analysis and
represent the keys of a dictionary and their corresponding types.
They are a useful tool when providing type hints of a JSON payload or
similar.

For example the following code snippet will raise a false-positive alert
from this flake8 plug-in:

class ApiPage(TypedDict):
  url: str
  next: str
  data: list[dict[str, Any]]

This is because a TypedDict is analysed as if it was a standard class.
Since it is only used in the type system it is safe to ignore anything
that inherits from TypedDict in this plugin.

Currently, this PR has the limitation that it does not handle inheritances.
There is also a foreseen issue where false positives will still occur if
the shadowing variable is in a class that inherits from a TypedDict defined
in another class.

`TypedDicts` are used only within the type system for analysis and
represent the keys of a dictionary and their corresponding types.
They are a useful tool when providing type hints of a JSON payload or
similar.

For example the following code snippet will raise a false-positive alert
from this flake8 plug-in:
```python
class ApiPage(TypedDict):
  url: str
  next: str
  data: list[dict[str, Any]]
```

This is because a `TypedDict` is analysed as if it was a standard class.
Since it is only used in the type system it is safe to ignore anything
that inherits from `TypedDict` in this plugin.
@danjones1618 danjones1618 marked this pull request as draft June 1, 2024 11:52
Copy link
Owner

@gforcada gforcada left a comment

Choose a reason for hiding this comment

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

Nice work! 🌟

Maybe it's too late and I'm a bit tired, but I fail to see what's the problem with the N-deep examples, but as they are guarded with an xfail that would not be too much of a problem 👍🏾

As the PR is a draft I'm not approving.

Ping me whenever the PR is ready... what's missing ? 🤔

Again, thanks for the work!! 🙇🏾

@@ -140,7 +140,9 @@ def run(self):

def check_assignment(self, statement):
msg = self.assign_msg
if type(statement.__flake8_builtins_parent) is ast.ClassDef:
if isinstance(statement.__flake8_builtins_parent, ast.ClassDef):
if "TypedDict" in {a.id for a in statement.__flake8_builtins_parent.bases if isinstance(a, ast.Name)}:
Copy link
Owner

Choose a reason for hiding this comment

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

@danjones1618 do we expect other typing classes to be used eventually here? 🤔

Given how long this conditional is, it might be worth refactoring into a function?

def base_classes_ids(self, statement):
    return {a.id for a in statement.__flake8_builtins_parent.bases if isinstance(a, ast.Name)}

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