Skip to content

Conversation

phofl
Copy link
Member

@phofl phofl commented Dec 22, 2021

  • Ensure all linting tests pass, see here for how to run them

@phofl phofl added the Typing type annotations, mypy/pyright type checking label Dec 22, 2021
@jreback
Copy link
Contributor

jreback commented Dec 27, 2021

looks fine can you rebase again

@phofl
Copy link
Member Author

phofl commented Dec 28, 2021

Done

except StopIteration:
self.buffer = None
line = next(self.f)
line = next(self.f) # type: ignore[arg-type]
Copy link
Member Author

Choose a reason for hiding this comment

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

@twoertwein Does ReadCsvBuffer suport next in addition to iter?

Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand: As long as .__iter__() returns an object that has __next__, I don't think the file object itself has to have __next__.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm weird, mypy complains about ReadCsvBuffer[str]

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, you are right! I think only read_fwf calls next explicitly, or? read_csv does not explicitly call next. The following works with read_csv

from io import StringIO

import pandas as pd


class Test:
    def __init__(self):
        self.buffer = StringIO("a,b,c\n0,1,2")

    @property
    def mode(self):
        return self.buffer.mode

    def fileno(self):
        return self.buffer.fileno()

    def seek(self, __offset, __whence=-1):
        if __whence != -1:
            return self.seek(__offset)
        return self.seek(__offset, __whence=__whence)

    def seekable(self):
        return self.buffer.seekable()

    def tell(self):
        return self.buffer.tell()

    def read(self, __n=None):
        self.buffer.read(__n)

    def __iter__(self):
        return self.buffer.__iter__()

    def readline(self):
        return self.buffer.readline()

    @property
    def closed(self):
        return self.buffer.closed()


print(pd.read_csv(Test(), engine="python"))

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep I think this is only called from read_fwf.

Can we add next to ReadCsvBuffer without breaking anything (from a correctness standpoint)

Copy link
Member

Choose a reason for hiding this comment

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

Would be fine with adding it or (probably better) creating ReadFwfBuffer which inherits from ReadCsvBuffer (not sure whether it needs everything that is in ReadCsvBuffer).

Also happy to have the ignores for now and address this in a later PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lets go with the ignore for now then until we have typed more. Can probably make a better decision then

@mroeschke mroeschke added this to the 1.5 milestone Feb 6, 2022
@mroeschke mroeschke merged commit 4558e7d into pandas-dev:main Feb 6, 2022
@mroeschke
Copy link
Member

Thanks @phofl

phofl added a commit to phofl/pandas that referenced this pull request Feb 14, 2022
* TYP: Type python parser

* Fix bug

* Fix assignment issue

* Adress conflicts

* Remove unnecessary changes

* Adjust
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
* TYP: Type python parser

* Fix bug

* Fix assignment issue

* Adress conflicts

* Remove unnecessary changes

* Adjust
@phofl phofl deleted the typ_python_parser branch August 17, 2022 20:03
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