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

Cleanup Typing in pandas.core.strings #25904

Merged
merged 8 commits into from
Apr 3, 2019
Merged

Conversation

WillAyd
Copy link
Member

@WillAyd WillAyd commented Mar 28, 2019

@WillAyd WillAyd added the Typing type annotations, mypy/pyright type checking label Mar 28, 2019
pandas/core/strings.py Show resolved Hide resolved
pandas/core/strings.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #25904 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25904      +/-   ##
==========================================
+ Coverage   91.53%   91.53%   +<.01%     
==========================================
  Files         175      175              
  Lines       52808    52810       +2     
==========================================
+ Hits        48338    48340       +2     
  Misses       4470     4470
Flag Coverage Δ
#multiple 90.1% <100%> (ø) ⬆️
#single 41.83% <90%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/strings.py 98.59% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 882961d...2dddb6e. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #25904 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25904      +/-   ##
==========================================
- Coverage    91.8%    91.8%   -0.01%     
==========================================
  Files         174      174              
  Lines       52548    52550       +2     
==========================================
- Hits        48243    48242       -1     
- Misses       4305     4308       +3
Flag Coverage Δ
#multiple 90.35% <100%> (ø) ⬆️
#single 41.87% <90%> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/core/strings.py 98.59% <100%> (ø) ⬆️
pandas/io/gbq.py 75% <0%> (-12.5%) ⬇️
pandas/core/frame.py 96.79% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.73% <0%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb89bc0...33c2278. Read the comment docs.

@jreback jreback added this to the 0.25.0 milestone Mar 28, 2019
version='')
_shared_docs['casefold'] = dict(type='be casefolded', method='casefold',
version='\n .. versionadded:: 0.25.0\n')
_doc_args = {} # type: Dict[str, Dict[str, str]]
Copy link
Contributor

Choose a reason for hiding this comment

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

you renamed this? does this still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

So to be clear I didn't rename this variable I just created a new one to clarify usage and types. _shared_docs still exists, but with str types for both keys and values.

It had mixed usage before because some values were also dicts and were being used to update other strings, so somewhat confusing as to the purpose of the variable and was causing typing to fail since the % operator can't have a dict as an operand

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a screenshot of the output of lower locally, which is of the items that was moved to the new variable here:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't make any sense, why did you change the name?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear I didn't change the name. _shared_docs still exists but now with just a type of Dict[str, str] so that it only holds docstrings.

Previously the signature was Dict[str, Union[Dict[str, str]]] because it was holding not only docstrings but also dicts to use for substitution in other docstrings, which was rather convoluted and was failing typing.

Instead of mashing all of those types into one _shared_docs variable it is arguably cleaner to have one dict containing docstrings (_shared_docs) and one containing substitution parameters (here _doc_args).

Copy link
Contributor

Choose a reason for hiding this comment

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

ok i see what you did. can you put a comment to reflect this fact

version='')
_shared_docs['casefold'] = dict(type='be casefolded', method='casefold',
version='\n .. versionadded:: 0.25.0\n')
_doc_args = {} # type: Dict[str, Dict[str, str]]
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't make any sense, why did you change the name?

version='')
_shared_docs['casefold'] = dict(type='be casefolded', method='casefold',
version='\n .. versionadded:: 0.25.0\n')
_doc_args = {} # type: Dict[str, Dict[str, str]]
Copy link
Contributor

Choose a reason for hiding this comment

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

ok i see what you did. can you put a comment to reflect this fact

@WillAyd
Copy link
Member Author

WillAyd commented Apr 3, 2019

@jreback ping on this one - any other changes needed?

@jreback jreback merged commit 1172d61 into pandas-dev:master Apr 3, 2019
@jreback
Copy link
Contributor

jreback commented Apr 3, 2019

thanks @WillAyd nope this one is good

@WillAyd WillAyd deleted the string-typing branch January 16, 2020 00:34
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.

Fix Type Annotations in pandas.core.strings
2 participants