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

Improve namespace package support #111

Closed theacodes closed 6 years ago

theacodes commented 7 years ago

We're using flake8 with namespace packages and we'd like to group our imports like this:

import functools

import google.auth  # separate package
from google.cloud import core  # separate package
import six

from google.cloud.bigquery import table  # package-local, we're in google/cloud/bigquery/job.py

It's fine if we need to specify this via a flag (like --application-import-names=google.cloud.bigquery)

Presently we must either choose between putting all google.* imports in the 2nd grouping or the 3rd grouping.

dhermes commented 7 years ago

For reference, these are the 3 packages (i.e. they really are all different):

sigmavirus24 commented 7 years ago

An aside, namespace packages can cause all sorts of headaches both for end-users and developers of those packages. Last I checked, the PyPA was generally recommending people not use namespace packages.

Does using --application-import-names=google.cloud.bigquery not fix this? I would think it would.

theacodes commented 7 years ago

Does using --application-import-names=google.cloud.bigquery not fix this? I would think it would.

It does not. I think it's because flake8 only considers the 'root' package (google) when determining the group.

An aside, namespace packages can cause all sorts of headaches both for end-users and developers of those packages

Other than some bumps at the beginning, they've mostly been fine for end-users. Developer tooling has been fun though. :)

Last I checked, the PyPA was generally recommending people not use namespace packages.

We (PyPA) now have comprehensive documentation on namespace packages. :) https://packaging.python.org/guides/packaging-namespace-packages/

sigmavirus24 commented 7 years ago

I think it's because flake8 only considers the 'root' package (google) when determining the group.

Meaning you have one large repository with a flake8 config at the top and then config for each sub-package? If so, I'm hoping to start being able to support this soonish.

theacodes commented 7 years ago

That's not the case right now (they all live in one repo, but are independent packages). I think what's going on is that flake8 reduces this:

import functools

import google.auth  # separate package
from google.cloud import core  # separate package
import six

from google.cloud.bigquery import table

to this:

first = ['functools']

second = ['google', 'google', 'six']

third = ['google']

And by that logic things are grouped wrong and setting --application-import-names=google.cloud won't work.

(I could be totally wrong)

sigmavirus24 commented 7 years ago

Oh, I thought you mean flake8 the top-level tool that provides information to plugins, not flake8 with flake8-import-order. I see what you're saying now.

pgjones commented 7 years ago

This plugin does only consider the root as the package name, so all the google imports are considered as the google package as you've noted as deduced here. I've spent some time looking and it isn't clear to me how you can tell if a package is a namespace package (maybe import it and look for a lack of __file__?). Do you have any ideas?

taion commented 7 years ago

Should this necessarily be specific to namespace packages anyway? It'd seem a little odd if specifying package.subpackage only worked if it were a namespace package.

sigmavirus24 commented 7 years ago

@taion I think it should. Consider the fact that most other applications/libraries/etc. aren't going to work the same way as a namespaced package might. If you own google.cloud.bigquery in a non-namespaced package then your application/library/etc. is almost always google otherwise. The only time that isn't necessarily the package is if it's a namespaced package.

taion commented 7 years ago

It seems like a bit of an odd restriction. I was actually surprised to find that this sort of thing didn't work.

We have a package we use for ML research. It's broken out into my_research.common and my_research.projects (e.g. my_research.projects.foo, my_research.projects.bar, where projects don't share code with each other, but do share common code). The whole thing ships as a single package, but I'd prefer to use something like the appnexus style and treat modules from my_research.common as if they were organization-local rather than package-local.

We may yet break this up into actual separate packages, and this is admittedly a bit of a weird edge case in package organization, but I'd be pretty confused if I saw that specifying package.subpackage only worked for namespace packages.

sigmavirus24 commented 7 years ago

The whole thing ships as a single package, but I'd prefer to use something like the appnexus style and treat modules from my_research.common as if they were organization-local rather than package-local.

I'm not familiar with the appnexus style, but if you don't want that to be considered local, why not ship it as a separate module within the same package? I think we're shaving hairs here, but in your case, would you want to specify --application-import-names as my_reserach.projects.{foo,bar,bogus}?

theacodes commented 7 years ago

I'm also happy to have to manually specify the --application-import-names to note which subpackages of a namespace package are 'local' and which are 'global'. I don't think flake8-import-order needs to do any special stuff to detect a namespace package.

taion commented 7 years ago

I'd want to specify something like:

application-import-names = my_research.projects
application-package-names = my_research.common

The idea here is more that, from a config perspective, I don't see why I'd have to think about namespace packages differently at all. That seems to me more of a question of packaging that, from the perspective of linting, should be opaque to me – rather I care about the logical package layout.

pgjones commented 6 years ago

Could you check if #126 fixes these issues?

pgjones commented 6 years ago

Fixed in master.