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

Make src_paths behave as expected when using --resolve-all-configs and improve performance #2142

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sudowork
Copy link

@sudowork sudowork commented Jun 6, 2023

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.

This change also introduces performance improvements to find_all_configs by pruning as we walk the filesystem and other smaller performance enhancements.

Fixes #2045

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

sudowork commented Jun 6, 2023

One thing of note: This would be backwards incompatible for anyone depending on the behavior of resolving first-party src_paths from the project root or config root. However, my assumption is that the behavior this change introduces is what users would expect given the types of config files we look for (.isort.cfg, pyproject.toml, setup.cfg, tox.ini, .editorconfig). The only odd one out in my opinion would be .editorconfig, which supports having a non-root configuration; however, given the behavior of the --resolve-all-configs flag to only choose the closest config file, I opted to not handle this case for consistency and to avoid further complicating the code.

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 sudowork changed the title Make src_paths behave as expected when using --resolve-all-configs Make src_paths behave as expected when using --resolve-all-configs and improve performance Jun 7, 2023
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)
Copy link
Author

Choose a reason for hiding this comment

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

This is the main logic to fix the src_paths resolution.

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.

inconsistencies of known_first_party and --resolve-all-configs argument
1 participant