NotificationsYou must be signed in to change notification settings
Fork596
Star6.6k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

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

Open

sudowork wants to merge2 commits intoPyCQA:main

base:main

Choose a base branch

fromsudowork:issue/2045

Open

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

sudowork wants to merge2 commits intoPyCQA:mainfromsudowork:issue/2045

Conversation

Copy link

sudowork commentedJun 6, 2023•
edited
Loading

When using--resolve-all-configs, there is unexpected behavior in thatsrc_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 setsdirectory on theconfig_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 bysrc_paths.

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

Fixes#2045

Make src_paths behave as expected when using --resolve-all-configs

1e90580

When using `--resolve-all-configs`, there is unexpected behavior in that`src_paths` ends up resolving relative to the project root, whichdefaults to the current working directory. This results in first-partymodules being marked as third-party modules in the default case.Under the previous implementation, one possible workaround would be tospecify the relative path to config directory (e.g.`relative/path/to/configdir/src`). However, assuming that the mostcommon use of `--resolve-all-configs` is to support multiplesub-projects in the same repository/overall directory, this workaroundwould now require each sub-project to understand where it lives in thefilesystem.This change proposes a fix that sets `directory` on the `config_data` tobe the directory containing the used configuration file if not alreadyset. Downstream, this directory is then used to resolve the absolutepaths specified by `src_paths`.FixesPyCQA#2045

Copy link

Author

sudowork commentedJun 6, 2023•
edited
Loading

One thing of note: This would be backwards incompatible for anyone depending on the behavior of resolving first-partysrc_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.

sudowork mentioned this pull request

Jun 7, 2023

inconsistencies of known_first_party and --resolve-all-configs argument#2045

Open

Greatly improve performance of find_all_configs

b5f83ab

Avoid recursing into default skip files like .venv. Also avoid doing aniteration per config file type. Lastly, use scandir to improve file system walkperformance.The previous implementation would walk the entire tree, and iterate overeach config source type to test if the file exists. Instead, walk overexisting files and do a fast filter to remove candidates.

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

sudowork commented

Jun 7, 2023

View reviewed changes

isort/settings.py

		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

sudoworkJun 7, 2023

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 thesrc_paths resolution.

Copy link

sblask commentedNov 8, 2024

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--resolve-all-configs would solve this, but quite the opposite, when using it, neither works. I'd expect the folder with the config file to be set as root for the files in them. Sounds like this PR would solve this?