TYP: Remove pandas.io.stata from Typing Blacklist #25940

ryankarlos · 2019-03-31T05:53:19Z

closes #Remove pandas.io.stata from Typing Blacklist #25932
passes tests
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2019-03-31T06:27:18Z

Codecov Report

Merging #25940 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25940      +/-   ##
==========================================
- Coverage   91.81%   91.81%   -0.01%     
==========================================
  Files         175      175              
  Lines       52580    52580              
==========================================
- Hits        48278    48274       -4     
- Misses       4302     4306       +4

Flag	Coverage Δ
#multiple	`90.36% <ø> (ø)`	⬆️
#single	`41.9% <ø> (-0.08%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`75% <0%> (-12.5%)`	⬇️
pandas/core/frame.py	`96.79% <0%> (-0.12%)`	⬇️

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 de3a85c...1d96fec. Read the comment docs.

codecov · 2019-03-31T06:27:18Z

Codecov Report

Merging #25940 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25940      +/-   ##
==========================================
- Coverage   91.84%   91.83%   -0.01%     
==========================================
  Files         175      175              
  Lines       52517    52517              
==========================================
- Hits        48232    48228       -4     
- Misses       4285     4289       +4

Flag	Coverage Δ
#multiple	`90.39% <ø> (ø)`	⬆️
#single	`40.72% <ø> (-0.14%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`75% <0%> (-12.5%)`	⬇️
pandas/core/frame.py	`96.79% <0%> (-0.12%)`	⬇️

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 9d66bdf...72a1702. Read the comment docs.

ryankarlos · 2019-03-31T14:32:41Z

@WillAyd Is it necessary to remove the method data_label which shadows the instance variable as reported in #25932 - removing it is making the docstring checks fail in the Azure Pipelines- associated with StataReader not having a data_label attribute ?

WillAyd · 2019-03-31T15:20:02Z

@ryankarlos what failure are you getting when removing?

ryankarlos · 2019-03-31T19:57:26Z

@ryankarlos what failure are you getting when removing?

I think the error was linked to validate_docstrings.py which parses the doc/source/reference/io.rst file to do the docstring validation on all the methods in io - there is a reference to StataReader.data_label there, so it is throwing an error 'StataReader' has no attribute 'data_label' because I had removed this method. Should I just remove it from this reference file as well - not sure about any other dependencies on this ?

ryankarlos · 2019-04-01T20:27:58Z

@WillAyd All green now after the fix.

WillAyd · 2019-04-02T01:09:06Z

Hmm not sure this is the right solution - would it actually make more sense to assign to self._data_label instead and keep the existing method?

jreback · 2019-04-02T12:41:33Z

cc @bashtage

jreback

this needs to be a private attribute (_data_label) I think.

bashtage · 2019-04-02T12:48:15Z

If you don't want to change the API, then switch self.data_label = _self._get... to self._data_label = _self._get... and then leave the method.

TBH it might as well be an attribute or a property, but this would break an API that is probably almost never used.*

Break it and you'll find someone who uses it I'm sure.

ryankarlos · 2019-04-02T18:18:23Z

Thanks, makes sense - ill push the changes

WillAyd

There are still a few other assignments to self.data_label in the module - can you check them all?

pandas/tests/io/test_stata.py

bashtage · 2019-04-03T23:31:24Z

The attribute seems to have precedence over the method, and so the public attribute should be retained and the method deleted.

ryankarlos · 2019-04-03T23:44:21Z

@bashtage just to clarify, are you saying better to revert back to the previous solution i.e switching back to self.data_label assignment in the module ?

bashtage · 2019-04-03T23:49:05Z

Yes. I just tried in master and Stata reader has an accessible attribute, not a method. You should not need to change the tests (and the api) to add typing.

…

On Thu, Apr 4, 2019, 00:44 Ryan Nazareth ***@***.***> wrote: @bashtage <https://github.com/bashtage> just to clarify, are you saying better to revert back to the previous solution i.e switching back to self.data_label assignment ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25940 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFU5RQ9xDV2-5eutwRsFwaNo6kLxQiXTks5vdTzwgaJpZM4cUIra> .

WillAyd · 2019-04-04T01:20:29Z

The method is documented as part of the API:

http://pandas.pydata.org/pandas-docs/stable/reference/io.html#stata

I agree that would be strange to use when compared against the attribute, but at the same time the method is what is exposed / documented so changing that would require a lot more effort / deprecation

bashtage · 2019-04-04T06:52:07Z

@WillAyd The documentation is wrong and should be fixed. Any user who is using this feature will get their code broken by a move to a method, and so introducing a method is a defacto API change.

from pandas.io.stata import StataReader

sr = StataReader('pandas/tests/io/data/stata14_118.dta')
print('Attribute')
print(sr.data_label)
print('Method')
print(sr.data_label())

produces

Attribute
This is a  Ünicode data label
Method
Traceback (most recent call last):
  File "C:\Anaconda\lib\site-packages\IPython\core\interactiveshell.py", line 3296, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-5-bce88bdbdeb6>", line 7, in <module>
    print(sr.data_label())
TypeError: 'str' object is not callable

WillAyd · 2019-04-04T06:56:31Z

Hmm OK interesting - thanks for confirming that. I suppose this is a strange in between of a instance variable and method which are two distinct things but probably should have been managed via a property. Given the instance variable is populate from a private method I suppose it's not supposed to be exposed as part of the API right?

bashtage · 2019-04-04T06:58:45Z

data_label should be part of the API since it could be useful to an end user. It could be a property (read-only), which a doc string, which would keep it in the API docs.

bashtage

A simple solution is to keep private attribute changes and then the switch data label to a property.

pandas/tests/io/test_stata.py

jreback

aside from the @bashtage changes, pls explain why we cannot leave the

from pandas import .....

I find this easier to read.

ryankarlos · 2019-04-04T21:20:43Z

Does this need to be reverted as well ? Was of the impression that the requirements in this issue were to explicitly import as defined internally ? Happy to revert though if not the case

aside from the @bashtage changes, pls explain why we cannot leave the

from pandas import .....

I find this easier to read.

bashtage

LGTM

…m mypy.ini blacklist

… error" This reverts commit 9ae0804.

ryankarlos · 2019-04-05T00:29:40Z

Not sure why CI is failing.

…ping_fix_pandas.io.stata

ryankarlos · 2019-04-05T22:13:17Z

@WillAyd All green now

WillAyd · 2019-04-06T04:30:41Z

Great thanks! I think we need a precursor to this PR to explicitly import objects in the top level of the code base instead of changing the import machinery here - any interest in tackling that?

ryankarlos · 2019-04-06T09:52:21Z

Sure, but would this be for the entire codebase or just this module ? i may need some help if i get stuck (the import machinery is more complicated than i thought it would be). just to clarify , do you want me to revert the import changes back to from pandas import ..... in this PR ? I also wanted to work through some of the mpy blacklist in #25882 - is that then dependant on the PR you are suggesting ?

WillAyd · 2019-04-06T15:30:45Z

Sure, but would this be for the entire codebase or just this module

I think OK to start with just the module and we can iterate from there to get more as needed. Starting small is always better and can expand as needed from there

do you want me to revert the import changes back to from pandas import ..... in this PR ?

Yes but after the precursor of step one is done

I also wanted to work through some of the mpy blacklist in #25882 - is that then dependant on the PR you are suggesting

So the precursor will only affect modules that throw an error like "Module 'pandas' has no attribute 'XXX'" - you can certainly still work on other modules without that error and they will be unaffected

Just a heads up I have limited availability until Tuesday so I can help with little things here and there but may be delayed in responding

ryankarlos · 2019-04-07T02:29:51Z

Ok thanks for the info, I have just made a PR #26017

Sure, but would this be for the entire codebase or just this module

I think OK to start with just the module and we can iterate from there to get more as needed. Starting small is always better and can expand as needed from there

do you want me to revert the import changes back to from pandas import ..... in this PR ?

Yes but after the precursor of step one is done

I also wanted to work through some of the mpy blacklist in #25882 - is that then dependant on the PR you are suggesting

So the precursor will only affect modules that throw an error like "Module 'pandas' has no attribute 'XXX'" - you can certainly still work on other modules without that error and they will be unaffected

Just a heads up I have limited availability until Tuesday so I can help with little things here and there but may be delayed in responding

jbrockmendel · 2019-04-08T18:25:07Z

@ryankarlos can you merge master

…a,io PRs #25936 #25940 #25939 (#26019)

…ping_fix_pandas.io.stata

jreback · 2019-04-10T12:59:48Z

thanks @ryankarlos

WillAyd added the Typing type annotations, mypy/pyright type checking label Mar 31, 2019

ryankarlos force-pushed the Typing_fix_pandas.io.stata branch from d2a8770 to 1d96fec Compare March 31, 2019 19:45

jreback requested changes Apr 2, 2019

View reviewed changes

WillAyd requested changes Apr 3, 2019

View reviewed changes

bashtage reviewed Apr 3, 2019

View reviewed changes

pandas/tests/io/test_stata.py Outdated Show resolved Hide resolved

bashtage requested changes Apr 4, 2019

View reviewed changes

pandas/tests/io/test_stata.py Outdated Show resolved Hide resolved

pandas/tests/io/test_stata.py Outdated Show resolved Hide resolved

jreback requested changes Apr 4, 2019

View reviewed changes

bashtage approved these changes Apr 4, 2019

View reviewed changes

ryankarlos added 4 commits April 5, 2019 00:36

Add adbsolute imports, removed data_label method and remove stata fro…

5cfa601

…m mypy.ini blacklist

Remove StataReader.data_label method from api rst file to fix error

e9c27b9

Revert "Remove StataReader.data_label method from api rst file to fix…

c410e6f

… error" This reverts commit 9ae0804.

Making instance variable private and adding back data_label method

f9a037e

ryankarlos added 2 commits April 5, 2019 00:36

modifying more assignments to self.data_label to pass tests

8a1e560

Make Stata.data_label method a property

4a58142

ryankarlos force-pushed the Typing_fix_pandas.io.stata branch from df8beec to 4a58142 Compare April 4, 2019 23:49

Merge branch 'master' of https://github.com/pandas-dev/pandas into Ty…

deccb63

…ping_fix_pandas.io.stata

ryankarlos mentioned this pull request Apr 7, 2019

Add explicit imports in top level pandas init as precursor to PR #25940 #26017

Closed

4 tasks

ryankarlos mentioned this pull request Apr 7, 2019

TYP: Replace wildcardimports in toplevelinit as precursor for reshape,stata,io PRs #25936 #25940 #25939 #26019

Merged

4 tasks

Merge master and fix conflict

b4197ce

jreback pushed a commit that referenced this pull request Apr 10, 2019

Replace wildcardimports in toplevelinit as precursor for reshape,stat…

9d66bdf

…a,io PRs #25936 #25940 #25939 (#26019)

ryankarlos added 2 commits April 10, 2019 02:24

Merge branch 'master' of https://github.com/pandas-dev/pandas into Ty…

362629c

…ping_fix_pandas.io.stata

Import from top level pandas

02bdfe5

WillAyd approved these changes Apr 10, 2019

View reviewed changes

Sorting imports with isort

72a1702

jreback added this to the 0.25.0 milestone Apr 10, 2019

jreback approved these changes Apr 10, 2019

View reviewed changes

jreback merged commit 6bb9ab8 into pandas-dev:master Apr 10, 2019

WillAyd mentioned this pull request Apr 10, 2019

Typing Cleanup - Remove Blacklisted Items #25882

Closed

65 tasks

ryankarlos deleted the Typing_fix_pandas.io.stata branch April 13, 2019 19:54

ryankarlos changed the title ~~Remove pandas.io.stata from Typing Blacklist~~ TYP: Remove pandas.io.stata from Typing Blacklist Jan 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TYP: Remove pandas.io.stata from Typing Blacklist #25940

TYP: Remove pandas.io.stata from Typing Blacklist #25940

ryankarlos commented Mar 31, 2019 •

edited

Loading

codecov bot commented Mar 31, 2019

codecov bot commented Mar 31, 2019 •

edited

Loading

ryankarlos commented Mar 31, 2019 •

edited

Loading

WillAyd commented Mar 31, 2019

ryankarlos commented Mar 31, 2019 •

edited

Loading

ryankarlos commented Apr 1, 2019

WillAyd commented Apr 2, 2019

jreback commented Apr 2, 2019

jreback left a comment

bashtage commented Apr 2, 2019

ryankarlos commented Apr 2, 2019

WillAyd left a comment

bashtage commented Apr 3, 2019

ryankarlos commented Apr 3, 2019 •

edited

Loading

bashtage commented Apr 3, 2019 via email

WillAyd commented Apr 4, 2019

bashtage commented Apr 4, 2019

WillAyd commented Apr 4, 2019

bashtage commented Apr 4, 2019

bashtage left a comment

jreback left a comment

ryankarlos commented Apr 4, 2019 •

edited

Loading

bashtage left a comment

ryankarlos commented Apr 5, 2019 •

edited

Loading

ryankarlos commented Apr 5, 2019 •

edited

Loading

WillAyd commented Apr 6, 2019

ryankarlos commented Apr 6, 2019

WillAyd commented Apr 6, 2019

ryankarlos commented Apr 7, 2019

jbrockmendel commented Apr 8, 2019

jreback commented Apr 10, 2019

TYP: Remove pandas.io.stata from Typing Blacklist #25940

TYP: Remove pandas.io.stata from Typing Blacklist #25940

Conversation

ryankarlos commented Mar 31, 2019 • edited Loading

codecov bot commented Mar 31, 2019

Codecov Report

codecov bot commented Mar 31, 2019 • edited Loading

Codecov Report

ryankarlos commented Mar 31, 2019 • edited Loading

WillAyd commented Mar 31, 2019

ryankarlos commented Mar 31, 2019 • edited Loading

ryankarlos commented Apr 1, 2019

WillAyd commented Apr 2, 2019

jreback commented Apr 2, 2019

jreback left a comment

Choose a reason for hiding this comment

bashtage commented Apr 2, 2019

ryankarlos commented Apr 2, 2019

WillAyd left a comment

Choose a reason for hiding this comment

bashtage commented Apr 3, 2019

ryankarlos commented Apr 3, 2019 • edited Loading

bashtage commented Apr 3, 2019 via email

WillAyd commented Apr 4, 2019

bashtage commented Apr 4, 2019

WillAyd commented Apr 4, 2019

bashtage commented Apr 4, 2019

bashtage left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

ryankarlos commented Apr 4, 2019 • edited Loading

bashtage left a comment

Choose a reason for hiding this comment

ryankarlos commented Apr 5, 2019 • edited Loading

ryankarlos commented Apr 5, 2019 • edited Loading

WillAyd commented Apr 6, 2019

ryankarlos commented Apr 6, 2019

WillAyd commented Apr 6, 2019

ryankarlos commented Apr 7, 2019

jbrockmendel commented Apr 8, 2019

jreback commented Apr 10, 2019

ryankarlos commented Mar 31, 2019 •

edited

Loading

codecov bot commented Mar 31, 2019 •

edited

Loading

ryankarlos commented Mar 31, 2019 •

edited

Loading

ryankarlos commented Mar 31, 2019 •

edited

Loading

ryankarlos commented Apr 3, 2019 •

edited

Loading

ryankarlos commented Apr 4, 2019 •

edited

Loading

ryankarlos commented Apr 5, 2019 •

edited

Loading

ryankarlos commented Apr 5, 2019 •

edited

Loading