Make src_paths behave as expected when using --resolve-all-configs and improve performance - #2142
Make src_paths behave as expected when using --resolve-all-configs and improve performance#2142sudowork wants to merge 2 commits into
Conversation
When using `--resolve-all-configs`, there is unexpected behavior in that `src_paths` ends up resolving relative to the project root, which defaults to the current working directory. This results in first-party modules being marked as third-party modules in the default case. Under the previous implementation, one possible workaround would be to specify the relative path to config directory (e.g. `relative/path/to/configdir/src`). However, assuming that the most common use of `--resolve-all-configs` is to support multiple sub-projects in the same repository/overall directory, this workaround would now require each sub-project to understand where it lives in the filesystem. This change proposes a fix that sets `directory` on the `config_data` to be the directory containing the used configuration file if not already set. Downstream, this directory is then used to resolve the absolute paths specified by `src_paths`. Fixes PyCQA#2045
|
One thing of note: This would be backwards incompatible for anyone depending on the behavior of resolving first-party This change also makes the behavior consistent with configuring a settings path. |
Avoid recursing into default skip files like .venv. Also avoid doing an iteration per config file type. Lastly, use scandir to improve file system walk performance. The previous implementation would walk the entire tree, and iterate over each config source type to test if the file exists. Instead, walk over existing files and do a fast filter to remove candidates.
sudowork
changed the title
| potential_config_file.path, CONFIG_SECTIONS[potential_config_file.name] | ||
| ) | ||
| if "directory" not in config_data: | ||
| config_data["directory"] = os.path.dirname(potential_config_file.path) |
There was a problem hiding this comment.
This is the main logic to fix the src_paths resolution.
|
I just ran into this problem. I have a mono repo with multiple python projects in sub folders that each have an .isort.cfg. When running isort from the root on a file in directory one, everything works as expected, same on a file in directory two. But when I run on a file from directory one and two at the same time (when using pre-commit), only one of the files is formatted correctly. I thought |
There was a problem hiding this comment.
Pull request overview
This PR fixes --resolve-all-configs behavior so src_paths resolves relative to the directory containing the config file selected for a given file (instead of the process CWD), addressing misclassification of first-party vs third-party imports (Fixes #2045). It also refactors config discovery to improve performance when scanning large trees.
Changes:
- Update
find_all_configsto scan viaos.scandir(with default-skip pruning) and populateconfig_data["directory"]from the config file’s directory when unset. - Extend unit tests for
find_all_configsto validate expectedsrc_pathsresolution and.venvskipping. - Update documentation to clarify
src_pathsresolution semantics under--resolve-all-configs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
isort/settings.py |
Refactors config discovery and injects directory into config data for correct src_paths resolution under --resolve-all-configs. |
tests/unit/test_settings.py |
Updates find_all_configs tests to validate src_paths resolution and skip-directory behavior. |
docs/configuration/options.md |
Clarifies --resolve-all-configs behavior for src_paths resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| config_sources_set = set(CONFIG_SOURCES) | ||
| for potential_config_file in _scanwalk_files( | ||
| path, exclude_fn=lambda entry: entry.name in DEFAULT_SKIP | ||
| ): | ||
| if potential_config_file.name not in config_sources_set: | ||
| continue | ||
| try: | ||
| config_data = _get_config_data( | ||
| potential_config_file.path, CONFIG_SECTIONS[potential_config_file.name] | ||
| ) | ||
| if "directory" not in config_data: | ||
| config_data["directory"] = os.path.dirname(potential_config_file.path) | ||
| trie_root.insert(potential_config_file.path, config_data) | ||
| except Exception: | ||
| warn(f"Failed to pull configuration information from {potential_config_file.path}") |
| def _scanwalk_files( | ||
| root_path: str, exclude_fn: Callable[[os.DirEntry], bool] = None | ||
| ) -> Iterator[os.DirEntry]: | ||
| # depth-first walk of file system starting with root_path | ||
| stack: List[str] = [root_path] | ||
| while stack: | ||
| dir_path = stack.pop() | ||
| with os.scandir(dir_path) as it: | ||
| for entry in it: | ||
| # Avoid processing excluded files/dirs and their descendants | ||
| if exclude_fn and exclude_fn(entry): | ||
| continue | ||
| if entry.is_dir(): | ||
| stack.append(entry.path) | ||
| elif entry.is_file(): | ||
| yield entry |
| pyproject_toml_file_broken = dir4 / "pyproject.toml" | ||
| pyproject_toml_file_broken.write_text(pyproject_toml_broken, "utf-8") | ||
|
|
||
| pyproject_toml_file_skip = dir_skip / "pyproject.toml" | ||
| pyproject_toml_file_skip.write_text(pyproject_toml, "utf-8") | ||
|
|
||
| config_trie = settings.find_all_configs(str(tmpdir)) |
When using
--resolve-all-configs, there is unexpected behavior in thatsrc_pathsends up resolving relative to the project root, which defaults to the current working directory. This results in first-party modules being marked as third-party modules in the default case.Under the previous implementation, one possible workaround would be to specify the relative path to config directory (e.g.
relative/path/to/configdir/src). However, assuming that the most common use of--resolve-all-configsis to support multiple sub-projects in the same repository/overall directory, this workaround would now require each sub-project to understand where it lives in the filesystem.This change proposes a fix that sets
directoryon theconfig_datato be the directory containing the used configuration file if not already set. Downstream, this directory is then used to resolve the absolute paths specified bysrc_paths.This change also introduces performance improvements to
find_all_configsby pruning as we walk the filesystem and other smaller performance enhancements.Fixes #2045