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

Issue with pre-commit hooks and src structure #214

Open
valentincalomme opened this issue Jan 16, 2024 · 2 comments · May be fixed by #216
Open

Issue with pre-commit hooks and src structure #214

valentincalomme opened this issue Jan 16, 2024 · 2 comments · May be fixed by #216

Comments

@valentincalomme
Copy link

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:

  • It's a bug; the pre-commit hook should work with this configuration, even if the code is in a src/ folder.
  • It's not a bug; if the code is in a src/ folder, .import-linter or .pre-commit-config.yaml need to be configured differently.

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
Copy link
Owner

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
Copy link
Author

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.

@sbrugman sbrugman linked a pull request Feb 23, 2024 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.

2 participants