PyCQA / flake8-import-order

Flake8 plugin that checks import order against various Python Style Guides
GNU Lesser General Public License v3.0
278 stars 72 forks source link

_csv is identified as Third Party instead of Stdlib #175

Open sanyassh opened 4 years ago

sanyassh commented 4 years ago

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 commented 4 years ago

_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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 2 years ago

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: https://github.com/urllib3/urllib3/issues/1071