seddonym / import-linter

Import Linter allows you to define and enforce rules for the internal and external imports within your Python project.
https://import-linter.readthedocs.io/
BSD 2-Clause "Simplified" License
692 stars 49 forks source link

Issue with pre-commit hooks and src structure #214

Open valentincalomme opened 10 months ago

valentincalomme commented 10 months ago

I've added import-linter to my pre-commit hooks for my copywriter project. I use a src/ folder structure where my source code is in src/copywriter and my tests are in tests/ in the root directory.

My .import-linter file looks like this:

[importlinter]
root_packages=
    copywriter
    tests

include_external_packages=True

[importlinter:contract:no-devtools-deps]
name = Application code should not import non-application code (i.e. tests, devtools)
type = forbidden
source_modules =
    copywriter
forbidden_modules =
    tests

and my pre-commit setup looks like this:

# Import-linter to check project structure
- repo: https://github.com/seddonym/import-linter/
  rev: "v2.0"
  hooks:
    - id: import-linter

I've noticed that even though running lint-imports directly works fine, running it via pre-commit returned the following error: Could not find package 'copywriter' in your Python path.. I figured it had to do with the src structure as if I move my copywriter folder to the top directory, the error disappears.

I am not sure if it's a bug or a feature, so to me, these are the options:

If it's the second, I am unsure of the preferred way. I've turned copywriter into src.copywriter in my .import-linter file, and it seems to have fixed the problem, but I'm not a fan.

If we resolve this issue, it would be wise to add something in the documentation, see my other issue.

seddonym commented 10 months ago

I've turned copywriter into src.copywriter in my .import-linter file, and it seems to have fixed the problem, but I'm not a fan.

I agree, I don't think that's the right fix. The error you're encountering is because copywriter isn't on your Python path - you could consider doing pip install -e . locally so it's available from your virtual environment.

More generally, I had a quick look locally and I couldn't reproduce the problem, but I am a bit unfamiliar with the development workflow for pre-commit hooks. I agree though it would be good to make the experience nicer, so happy to consider a pull request that sorts this out.

valentincalomme commented 10 months ago

I think I figured it out, pre-commit uses an isolated environment per hook, so I'd need to specify language = system or additional_dependencies to make sure the hook is aware of my package.

nathanjmcdougall commented 4 months ago

The way pre-commit is designed, it will try to create a managed virtual environment to run the tool. But these venvs are not configurable - e.g. you can't run pip install -e ., nor set PYTHONPATH. There are hacks e.g. entry but they won't be platform independent, e.g. Windows vs. Linux.

I think the best solution for pre-commit users would be a new configuration option where the user can specify a subdirectories in the project root in which to discover packages, in this case ["src"]. I assume these would be appended to sys.path internally.

The alternative option, in #216 is to use the system Python to run the tool. But this has its own drawbacks (for example it is not compatible with pre-commit.ci), and is strongly discouraged within the pre-commit framework.