Skip to content

Conversation

@simonjayhawkins
Copy link
Member

broken off #28339

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Nov 27, 2019
@jbrockmendel
Copy link
Member

small comments, non-blockers, LGTM

@simonjayhawkins simonjayhawkins added this to the 1.0 milestone Nov 27, 2019
@jbrockmendel
Copy link
Member

LGTM

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm. optional comment to maybe address or merge if you disagree

raise ValueError("{k} is not a valid identifier".format(k=k))
# NOTE: tokenize.Name is not a public constant
# error: Module has no attribute "Name" [attr-defined]
if not re.match("^" + tokenize.Name + "$", k): # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth considering tokenize.Name to \w+; it hasn't changed in 9 years:

https://github.com/python/cpython/blob/305189ecdc8322c22879a04564cad5989f937462/Lib/tokenize.py#L67

@jreback jreback merged commit 3a37464 into pandas-dev:master Nov 29, 2019
@jreback
Copy link
Contributor

jreback commented Nov 29, 2019

thanks @simonjayhawkins, @WillAyd comment might be worth a followup

@simonjayhawkins simonjayhawkins deleted the pandas/_config/config.py branch December 1, 2019 13:20
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@simonjayhawkins
Copy link
Member Author

thanks @simonjayhawkins, @WillAyd comment might be worth a followup

The ignore was removed in #36012, since Name was added to the stub for tokenize.

I don't think a follow-up now needed or we need to change our usage.

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

Labels

Typing type annotations, mypy/pyright type checking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants