fix: don't crash on --config-root without --resolve-all-configs#2597
Conversation
…onfigs config_root was only popped out of config_dict when resolve_all_configs was set, so using --config-root on its own left it in the dict that gets splatted into Config(**config_dict), which blows up with UnsupportedSettings. The help text for --config-root already says it's "used with the --resolve-all-configs flag", implying it's just a no-op otherwise, not a hard error. Pop it unconditionally instead. Fixes PyCQA#1990 Signed-off-by: Kyue <164024549+Gooh456@users.noreply.github.com>
DanielNoord
left a comment
There was a problem hiding this comment.
We should give a warning (by raising an error) that this config isn't useful without resolve_all_configs. See other examples in the codebase.
Signed-off-by: Kyue <164024549+Gooh456@users.noreply.github.com>
|
good call. added that, exits with a clear message now instead of the confusing UnsupportedSettings crash. same sys.exit("Error: ...") pattern used a few lines up for the no-paths case. updated the old test to expect the error and added one for config-root + resolve-all-configs together still working. |
| allow_root = config_dict.pop("allow_root", None) | ||
| resolve_all_configs = config_dict.pop("resolve_all_configs", False) | ||
| config_root_given = "config_root" in config_dict | ||
| config_root = config_dict.pop("config_root", ".") |
There was a problem hiding this comment.
This can remain on line 1051 now
Signed-off-by: Kyue <164024549+Gooh456@users.noreply.github.com>
|
yeah good catch, moved it back inline. no reason to hold it in a separate var anymore. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2597 +/- ##
==========================================
+ Coverage 99.30% 99.33% +0.03%
==========================================
Files 41 41
Lines 3150 3155 +5
Branches 680 682 +2
==========================================
+ Hits 3128 3134 +6
Misses 14 14
+ Partials 8 7 -1 🚀 New features to boost your workflow:
|
yeah this one's just been broken since forever.
--config-rooton its own throws a rawUnsupportedSettingstraceback instead of doing... anything.Root cause is in
main.py:config_rootonly gets popped out ofconfig_dictinside theif resolve_all_configs:branch. So if you pass--config-rootwithout also passing--resolve-all-configs, it's still sitting in the dict when it gets splatted intoConfig(**config_dict), andConfighas no idea whatconfig_rootis, so it errors out.The help text for the flag literally says it's "used with the --resolve-all-configs flag" - so without that flag it should just be a no-op, not a crash. Fix is to pop it unconditionally, same as
resolve_all_configsitself already is a few lines up.Verified against main: reverted my change locally and reproduced the exact traceback from #1990 (
isort.exceptions.UnsupportedSettings: ... - config_root = ..). With the fix it runs fine.Added a regression test in
tests/unit/test_main.pythat calls--config-rootwithout--resolve-all-configsand just checks it doesn't blow up.Fixes #1990