Allow TypeVar names to start with an underscore #246
Conversation
This is consistent with our CapWords-based N801 class name check, which shares a similar PEP 8 definition. Resolves #245
testsuite/N808.py
Outdated
| #: N808 | ||
| __NotGood = TypeVar("__NotGood") | ||
| #: Okay | ||
| __Good = TypeVar("__Good") |
There was a problem hiding this comment.
I disagree. I believe _T_co is fine, or _T, but I don't believe __T (double underscore) is a good example here.
There was a problem hiding this comment.
| __Good = TypeVar("__Good") | |
| _Good = TypeVar("_Good") | |
| #: N808 | |
| __NotGood = TypeVar("__NotGood") |
There was a problem hiding this comment.
I don't have strong feelings either way. I haven't had a case (personally) where I would use a leading underscore, but it appears others do this fairly often:
Leading double underscores exist but are uncommon, so I think that's a good argument against supporting them:
src/pep8ext_naming.py
Outdated
| yield self.err(body, 'N808', name=name) | ||
|
|
||
| if not name[:1].isupper(): | ||
| stripped_name = name.lstrip('_') |
There was a problem hiding this comment.
I think the simplicity here is the attractive nuisance. I would probably bail on if name.startswith("__") because again, what's being asked for in the original issue is a single leading underscore. Double leading underscores for this are something I think we should draw a firm line in the sand about. I also don't agree with _T_co but am fine supporting it if someone can justify why they want a typevar that starts like that (aside from they're telling people to from <mod> import * which is just a bad reason to justify this).
I think we can simplify a lot of this logic though, because in retrospect, we can do
if name.startswith('__'):
yield self.err(body, 'N808', name=name) # Maybe return to short-circuit things?
stripped_name = name.lstrip('_')
parts = stripped_name.rsplit('_', 1)
if not parts[0].istitle():
yield self.err(body, 'N808', name=name)
# etc.I don't think the __ is ever something we should not alert on frankly. It causes lots of silly magical things in the background.
There was a problem hiding this comment.
I also have some other overall improvements to this function that I'll save for another pull request.
jparise
changed the title
| yield self.err(body, 'N808', name=name) | ||
|
|
||
| if not name[:1].isupper(): | ||
| stripped_name = name.removeprefix('_') |
This is consistent with our CapWords-based N801 class name check, which shares a similar PEP 8 definition.
Resolves #245