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

N815: Ignore TypedDict class variable casing #189

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danielpatrickdotdev
Copy link

Prosed fix for #178

Add specific exemption from N815 for all subclasses of TypedDict because class variable naming conventions should not apply to dictionary keys.

I've tried to reuse code from the rule for Exceptions (N818) as per the suggestion on #178.

This is my first time working with the ast module or with any flake8 plugin so please bear that in mind when reviewing this PR. Any advice on whether it's reasonable to tag every ClassDef with the names of its superclasses, and potential performance concerns around that, would be very welcome. On balance, I thought it may be preferable to do this rather than determining the superclasses again for every variable in a class.

Thanks :)

@danielpatrickdotdev
Copy link
Author

Whilst this approach covers many scenarios, I can't see an easy way to handle mutliple layers of inheritance from TypedDict across modules. If anyone has any thoughts on that I would appreciate it!

@@ -448,6 +453,8 @@ class VariablesCheck(BaseASTCheck):
def _find_errors(self, assignment_target, parents, ignore):
for parent_func in reversed(parents):
if isinstance(parent_func, ast.ClassDef):
if "TypedDict" in parent_func.superclasses:
Copy link
Member

Choose a reason for hiding this comment

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

Why are we skipping this altogether? People may in fact want to check this. I would instead think we should add an error code for this case so it can be ignored/disabled on a case-by-case basis

Choose a reason for hiding this comment

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

Good question. Happy to have a go at implementing that. Would you go with something like N819: mixedCase variable in TypedDict? Do you think we should disable by default?

Choose a reason for hiding this comment

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

Have pushed a new commit to implement this new error code, let me know what you think

@sigmavirus24
Copy link
Member

Whilst this approach covers many scenarios, I can't see an easy way to handle mutliple layers of inheritance from TypedDict across modules. If anyone has any thoughts on that I would appreciate it!

We can't handle that at all. To do so would require running the code to get the __mro__ at runtime. As this is purely static analysis, we can't build that ourselves - it's too complex. We already have plenty of cases where we might otherwise want to ignore things like this and can't handle it for this exact same reason

@danielpatrickdotdev
Copy link
Author

We can't handle that at all. To do so would require running the code to get the __mro__ at runtime. As this is purely static analysis, we can't build that ourselves - it's too complex. We already have plenty of cases where we might otherwise want to ignore things like this and can't handle it for this exact same reason

I suspected as much but wasn't sure of what a solution might involve. Thanks

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