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

_csv is identified as Third Party instead of Stdlib #175

Open
sanyassh opened this issue Sep 23, 2020 · 8 comments · May be fixed by #198
Open

_csv is identified as Third Party instead of Stdlib #175

sanyassh opened this issue Sep 23, 2020 · 8 comments · May be fixed by #198

Comments

@sanyassh
Copy link

sanyassh commented Sep 23, 2020

Having this code:

from _csv import _reader as csv_reader, _writer as csv_writer
from logging import Logger

Produces the following error messages when running flake8 with flake8-import-order:

.\annotations.py:2:1: I100 Import statements are in the wrong order.
 'from logging import Logger' should be before 'from _csv import _reader, _writer' and in a different group.
.\annotations.py:2:1: I201 Missing newline between import groups.
 'from logging import Logger' is identified as Stdlib and 'from _csv import _reader, _writer' is identified as Third Party.

To my understanding, _csv is a stdlib module because csv is and _csv is it's "relative" written in C and flake8 has to recognize it as stdlib.

 flake8 --version
3.8.3 (assertive: 1.2.1, flake8-bugbear: 20.1.4, flake8-comprehensions: 3.2.3,
flake8_commas: 2.0.0, import-order: 0.18.1, mccabe: 0.6.1, pycodestyle: 2.6.0,
pyflakes: 2.2.0) CPython 3.6.1 on Windows
@sigmavirus24
Copy link
Member

_csv would be a private module that you're not intended to import directly. Instead, I expect that csv uses it under the covers and what you get from csv is derived from _csv. I'd argue this is doing the right thing by flagging inappropriate usage of that module (even if it's not obvious)

@sanyassh
Copy link
Author

I have to import _csv to make annotations like def write(writer: csv_writer) -> None:. I haven't found anything appropriate for annotations like this in usual csv module. Anyway, this is still an issue of flake8 not to recognize _csv correctly.

@sigmavirus24
Copy link
Member

I haven't found anything appropriate for annotations like this in usual csv module.

That's disappointing and surprising. I'm not sure that means you should be using _csv.

Anyway, this is still an issue of flake8 not to recognize _csv correctly.

Except it's not documented on docs.python.org so it's not something this plugin should recognize. There's nothing to stop this module from being rewritten in a memory safe language that eliminates _csv and then we're stuck attempting to recognize something that isn't present any longer.

@sanyassh
Copy link
Author

sanyassh commented Sep 24, 2020

So you're saying that recognizing _csv and other such modules like_collections, _decimal, _json, _random and so on as Third Party is correct behavior? At least they should be identified as StdlibC, for example. They're not third party modules in any sense.

I haven't found anything appropriate for annotations like this in usual csv module.

That's disappointing and surprising. I'm not sure that means you should be using _csv.

It's a known issue of csv module not exposing the class of csv.writer() result. This SO answer suggests to use _csv._writer: https://stackoverflow.com/a/51270295/9609843

@sigmavirus24
Copy link
Member

So you're saying that recognizing _csv and other such modules like_collections, _decimal, _json, _random and so on as Third Party is correct behavior? At least they should be identified as StdlibC, for example. They're not third party modules in any sense.

I'm saying they're not documented as the standard library. They're implementation details. Referring to them is an anti-pattern.

It's a known issue of csv module not exposing the class of csv.writer() result. This SO answer suggests to use _csv._writer: https://stackoverflow.com/a/51270295/9609843

That's probably one of many potential solutions here. But I recognize the rest aren't on StackOverflow so they're not immediately obvious. I'd argue that https://stackoverflow.com/a/51267141 is the better solution since it seems unlikely for mypy to be able to handle other CSV readers/writers that could satisfy the API.

I'd also argue this is more of a bug for typeshed/the stdlib for not providing something here.

@public
Copy link
Member

public commented Sep 24, 2020

Currently there are a handful of _private stdlib modules in the list. Maybe that's the real bug? :)

https://github.com/PyCQA/flake8-import-order/blob/master/flake8_import_order/stdlib_list.py#L42-L44

Except it's not documented on docs.python.org so it's not something this plugin should recognize

Originally there were no internal _ modules in the stdlib list but they seem to have got added over the many years since.

@sigmavirus24
Copy link
Member

@public that's something I would agree would be a bug. I know y'all aren't really working very much on this these days, but I think a compromise would be to allow people to "extend" what the canonical list of standard library modules is with a flag to Flake8. I'm happy to help out here although I can't commit to a specific timeframe of when that would get a PR or anything.

@jvanasco
Copy link

The same thing applies to _socket, which is the cPython c extension of socket.

Referring to them is an anti-pattern.

Utilizing them directly may be an anti-pattern, but referring to them is definitely not and required by various use-cases.

In my particular use-case, I must inspect the socket on the raw requests response's _connection or fp and ensure it's class inheritance from socket to extract the remote peer's ip. Under cpython, because of the implementation details, this will never be an instance of socket.socket, and will almost never be an instance of socket._socketobject, but would typically by an instance of _socket._socket. This ultimately traces back to urllib3, and this 5 year old thread I started there: urllib3/urllib3#1071

@Dreamsorcerer Dreamsorcerer linked a pull request Dec 3, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants