TYP:  Remove pandas.io.stata from Typing Blacklist
State: closed

- [x] closes ##25932
- [x] passes tests 
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=h1) Report
> Merging [#25940](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/de3a85c8404505ace0c0febc26b1fbe824f48034?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25940/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=tree)

```diff
@@            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% <ø> (ø)` | :arrow_up: |
| #single | `41.9% <ø> (-0.08%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25940/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `75% <0%> (-12.5%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25940/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.79% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=footer). Last update [de3a85c...1d96fec](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=h1) Report
> Merging [#25940](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/9d66bdf1c6f252ba8a9f7b3298877d9c021a84c9?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25940/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=tree)

```diff
@@            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% <ø> (ø)` | :arrow_up: |
| #single | `40.72% <ø> (-0.14%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25940/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `75% <0%> (-12.5%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25940/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.79% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=footer). Last update [9d66bdf...72a1702](https://codecov.io/gh/pandas-dev/pandas/pull/25940?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@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 ?
@ryankarlos what failure are you getting when removing?
> @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 ?   

@WillAyd All green now after the fix.
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?
cc @bashtage 
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. 
Thanks, makes sense - ill push the changes
The attribute seems to have precedence over the method, and so the public attribute should be retained and the method deleted.
@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 ? 

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 <notifications@github.com> 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
> <https://github.com/pandas-dev/pandas/pull/25940#issuecomment-479697272>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AFU5RQ9xDV2-5eutwRsFwaNo6kLxQiXTks5vdTzwgaJpZM4cUIra>
> .
>

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
@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
```

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?
`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. 
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.


Not sure why CI is failing.
@WillAyd All green now
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?
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 ? 
> 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
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


@ryankarlos can you merge master
thanks @ryankarlos