Skip to content

Feat/source roots option#323

Open
marktodisco wants to merge 5 commits into
seddonym:mainfrom
marktodisco:feat/source-roots-option
Open

Feat/source roots option#323
marktodisco wants to merge 5 commits into
seddonym:mainfrom
marktodisco:feat/source-roots-option

Conversation

@marktodisco

@marktodisco marktodisco commented Dec 20, 2025

Copy link
Copy Markdown

This pull request resolves #274 by introducing support for a new source_roots configuration option, allowing users to specify additional directories to be added to the Python import search path. This is particularly useful for projects using a src/ layout or monorepos with multiple package roots. The changes include updates to documentation, release notes, logic, and tests to ensure correct handling of the new option.

  • Add tests for the change.
  • Add any appropriate documentation.
  • Run just check. (see note below)
  • Add a summary of changes to docs/release_notes.md.
  • Add your name to docs/authors.md (in alphabetical order).

Note: The just check command as currently configured requests Python 3.9 which contradicts requires-python = ">=3.10" in pyproject.toml. I ran just check (excluding just test-all), and manually executed only the relevant tests with for v in 10 11 12 13 14; do just "test-3-$v"; done. Also, there is an issue with parallel test execution because it consistently failed on my machine.

@marktodisco

Copy link
Copy Markdown
Author

@seddonym can you please advise on which tests you feel are necessary and how they should be organized?

@marktodisco
marktodisco marked this pull request as ready for review December 21, 2025 17:42

@seddonym seddonym left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

It's looking pretty good, but prior to this it has always been possible to unit test use cases by configuring test doubles. We probably need to do something about the Python path in the same way. I'll have a think about the best way to handle it in this case and get back to you (should only be a small tweak).

@seddonym

seddonym commented Dec 22, 2025

Copy link
Copy Markdown
Owner

Actually I've realised we do something like this already in one of my other packages, Impulse.

https://github.com/seddonym/impulse/blob/main/tests/unit/application/test_use_cases.py#L68

Here, we pass sys.path into the function (optionally), allowing us to test it without mocking. Do you feel comfortable in making that change? If not I can do it.

@marktodisco

marktodisco commented Apr 30, 2026

Copy link
Copy Markdown
Author

@seddonym - I'm sorry for letting this go stale. Please let me know if you agree on the below points.

Given the merge conflict, I think it would be best for me to open a fresh PR with these changes, rather than try to detangle the conflicts.

And just to make sure I understand your ask. You would you like me to inject sys.path as a parameter to avoid mocks. Is that correct?

Current:

def _modify_path(user_options: UserOptions) -> None:

Preferred:

def _modify_path(user_options: UserOptions, sys_path: list[str] | None = None) -> None:

@seddonym

seddonym commented May 5, 2026

Copy link
Copy Markdown
Owner

Sorry for delay, had to refresh my memory on this too.

Looking a bit more closely I realize we have a different mechanism for injecting test doubles than we use for the library I shared with you - here, we have a Settings object that we configure with things we want to be able to swap. For the sake of consistency I think we should do the same thing here.

It should be simple - something along these lines:

   settings.configure(
       ...,
       SYS_PATH=sys.path,
       GET_CWD=os.getcwd,
    )

_modify_path can then use settings.SYS_PATH and settings.GET_CWD().

Rather than testing _modify_path directly, it would be greate if we could call the public function (importlinter.application.use_cases.lint_imports), and swap in test fakes using TestCheckContractsAndPrintReport._configure, as with the other ones. Hopefully that's clear, if not let me know.

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 this pull request may close these issues.

New configuration option to extend path

2 participants