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

Request: configurability of project root (for imports), or respecting flake8 --config as root #45

Open
jamesbraza opened this issue Sep 14, 2021 · 8 comments

Comments

@jamesbraza
Copy link
Contributor

I work at a company that uses native namespace packages for multiple internal packages.

Let's say our namespace is foo, so in a given package foo-ham we have import statements like:

from foo.bar import ...  # Import from internal package foo-bar
from foo.baz import ...  # Import from internal package foo-baz
from foo.spam import ...  # Import from internal package foo-spam

And in the foo-ham repo's setup.py we will have:

install_requires=[
    "foo-bar"
    "foo-baz",
    "foo-spam",
],

Currently, flake8-requirements reports:

I900 'foo' not listed as a requirement

How can I get flake8-requirements to properly understand native namespace packages?

@arkq
Copy link
Owner

arkq commented Sep 16, 2021

You will have to add custom mappings. See this: https://github.com/Arkq/flake8-requirements#customization

@jamesbraza
Copy link
Contributor Author

jamesbraza commented Oct 5, 2021

Okay I have added the known-modules to my setup.cfg like so:

# Map namespace packages to imports for flake8-requirements
# SEE: https://github.com/Arkq/flake8-requirements#customization
known-modules = foo-bar:[foo.bar],foo-baz:[foo.baz],foo-spam:[foo.spam]

In actuality, I have a lot of namespace repos, and this known-modules line is longer than 120 chars. Is there some way to line wrap this? Support for this syntax would be desirable:

known-modules =
    foo-bar:[foo.bar],
    foo-baz:[foo.baz],
    foo-spam:[foo.spam],

Also, one other question. My python package is inside in a src directory from the repo root. I invoke flake8 via pre-commit:

  - repo: "https://gitlab.com/pycqa/flake8"
    rev: 3.9.2
    hooks:
      - id: flake8
        name: flake8 src
        alias: flake8-src
        files: ^src/
        args:
          - "--config"
          - src/setup.cfg
        additional_dependencies:
          - flake8-docstrings
          - flake8-bugbear
          - flake8-requirements

Unfortunately, I find flake8-requirements doesn't work when invoked via pre-commit run -a. Notably, it doesn't respect the known-modules in setup.cfg and # noqa: I900 comments. Any ideas why?

src/pkg/pathj/to/file.py:5:1: I900 'foo' not listed as a requirement
src/pkg/path/to/file.py:7:1: I900 'foo' not listed as a requirement

It seems like the core problem is flake8-requirements needs to be invoked from the package root, and the actual package root is in src (not the repo root). Is there a way to notify flake8-requirements that the package isn't in the repo root?

The workaround for this repo root issue (per pre-commit/pre-commit#1417 (comment)) is to invoke flake8 manually, not through pre-commit's own installs:

  - repo: local
    hooks:
    -   id: flake8
        name: flake8 src package
        alias: flake8-src
        language: system
        entry: bash -c "cd src && flake8"

@arkq
Copy link
Owner

arkq commented Sep 14, 2022

Sorry for such a late reply, but somehow I've missed your comment... GitHub does not send notification when comment is edited :/

It seems like the core problem is flake8-requirements needs to be invoked from the package root, and the actual package root is in src (not the repo root). Is there a way to notify flake8-requirements that the package isn't in the repo root?

Yes, that might be the issue here... This plug-in checks for python project root directory on the first flake8 invocation. It starts searching for the root from the current working directory. But maybe that could be somehow mitigated by checking argument passed to flake8... I will have to check that.

In actuality, I have a lot of namespace repos, and this known-modules line is longer than 120 chars. Is there some way to line wrap this?

In setup.py it's not possible. But you can check recently merged PR #57

@arkq
Copy link
Owner

arkq commented Sep 15, 2022

I've made one check with such setup:

.
├── main.py             # this file contains line exceeding 50 characters
├── setup.cfg           # [flake8]: max-line-length = 50
└── subproject
    ├── extra.py        # this file contains line exceeding 30 characters but less then 50
    └── setup.cfg       # [flake8]: max-line-length = 30

Test:

$ flake8 
./main.py:1:51: E501 line too long (65 > 50 characters)
$ flake8 subproject
$ cd subproject
subproject $ flake8 
./extra.py:1:31: E501 line too long (37 > 30 characters)

So, even flake8 itself reads configuration from the current directory. For consistency, I think, flake8-requirements should do the same thing.

@jamesbraza
Copy link
Contributor Author

So, even flake8 itself reads configuration from the current directory. For consistency, I think, flake8-requirements should do the same thing.

This is close, but I think you're missing a detail. Note in my .pre-commit-config.yaml snippet above I am passing --config (see the args key). This --config means the rest of flake8 will work, but it leaves flake8-requirements behind.

So in your repro above, the 2nd line should pass --config:

> flake8
./main.py:1:51: E501 line too long (74 > 50 characters)
> flake8 --config subproject/setup.cfg subproject
subproject/extra.py:1:31: E501 line too long (46 > 30 characters)
> cd subproject && flake8
./extra.py:1:31: E501 line too long (46 > 30 characters)

So what can be done? Perhaps flake8-requirements should take the --config directory as the "root", not just default to the repo root. What do you think?

@arkq
Copy link
Owner

arkq commented Sep 16, 2022

Perhaps flake8-requirements should take the --config directory as the "root", not just default to the repo root. What do you think?

Hmm... maybe you are right. But I'm thinking of a case where the config for flake8 is kept separately from the repo/project itself. So, setting the root of the project based on config might lead to issues as well... I'm more inclined to search for project's root on every file scan (not only of flake8 startup). This will decrease performance, but I'm not sure how much... Alternatively, such approach (searching for project's root on every file scan) might be enabled on demand by flake8 option. I'll have to check that on some big python repo.

@jamesbraza
Copy link
Contributor Author

But I'm thinking of a case where the config for flake8 is kept separately from the repo/project itself.

That makes sense, good point. I agree it's not the right solution to assume the setup.cfg is in the same place as the package.

I'm more inclined to search for project's root on every file scan (not only of flake8 startup). This will decrease performance, but I'm not sure how much...

I also agree here, I don't like the possibility of a performance hit due to inference.

Perhaps another option is be adding an additional argument --package-root, --root-path, or --requirements-path (that can also be set in a setup.cfg file). That would avoid the performance hit and also solve this problem in a flexible manner.

@iivanov-qb
Copy link
Contributor

In actuality, I have a lot of namespace repos, and this known-modules line is longer than 120 chars. Is there some way to line wrap this? Support for this syntax would be desirable:

known-modules =
    foo-bar:[foo.bar],
    foo-baz:[foo.baz],
    foo-spam:[foo.spam],

I agree with this one. Is there a way to have multiline definition of known-modules?

@jamesbraza jamesbraza changed the title Support for native namespace packages? Request: configurability of project root (for imports), or respecting flake8 --config as root Feb 20, 2024
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

No branches or pull requests

3 participants