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

Config override key flag does not work for extend option #10035

Open
ofek opened this issue Feb 19, 2024 · 11 comments
Open

Config override key flag does not work for extend option #10035

ofek opened this issue Feb 19, 2024 · 11 comments
Assignees
Labels
cli Related to the command-line interface configuration Related to settings and configuration

Comments

@ofek
Copy link
Contributor

ofek commented Feb 19, 2024

Example command:

ruff format --check --diff --config "extend='ruff_defaults.toml'" .
@MichaReiser MichaReiser added configuration Related to settings and configuration cli Related to the command-line interface labels Feb 19, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Feb 19, 2024

This is an interesting usage of the new override system and should be supported. However, It may not have the desired result depending on your use case:

pyproject.toml

tests\
	tests.config.toml // extends pyproject.toml

Overriding the extend would override the extend value for all configurations, meaning that tests.config.toml would suddenly no longer extend the pyproject.toml. Now, that might be exactly what you want but may be unintentional if the intention was to make all configurations inherit from a base configuration.

CC: @AlexWaygood

@AlexWaygood AlexWaygood self-assigned this Feb 19, 2024
@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 19, 2024

Ah I see -- so, I think this doesn't work because we currently apply the configuration overrides given in the --config flag after any configuration options found in any pyproject.toml/ruff.toml files have been parsed and validated. But this particular override affects which config files ruff should be looking at in the first place, so we'd need to look for it at an earlier stage in the process (I think).

I'm unlikely to have time to look at this in any more depth this week, unfortunately, but I'll work on this as soon as I'm able. Thanks for the report @ofek!

@ofek
Copy link
Contributor Author

ofek commented Feb 19, 2024

I'm okay with a fix or literally any other way to define an extend via command line! If it means a separate flag then that's cool.

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 19, 2024

Yeah, I'll have a think...

If we decide that this kind of thing should be done via a separate flag, then we should probably explicitly reject trying to override extend via --config, on the grounds that it's probably not going to do what you expect, as @MichaReiser mentioned above. So whether we go with "fix behaviour of --config w.r.t. extend" or "introduce another flag", it's likely the behaviour of --config should be changed in some way here :)

@QuentinSoubeyranAqemia
Copy link

+1 on this feature
This would be helpful to extend a base configuration with more specific overrides, when the path to the base config is not static and thus cannot be hard-coded into the more-specific configuration.

If that is any indication, when searching the doc I was also expecting a dedicated --extend flag, and tried out the --config override after failing to find one.
Given that --config "KEY = VALUE" has highest priority and thus needs to be applied last, it makes sense to me that --config cannot support --extend behavior as is currently the case.

I suppose a tricky part could be extend chains: with extend in the files, you can only have a linear chain of files extending one another, so the behavior is well-defined. No diamond inheritance.
With both the file passed to --config, and --extend, possibly extending another file, there can be multiple inheritance. This could reasonably result in an error unless a use-case pops up, though?

@zanieb
Copy link
Member

zanieb commented Mar 15, 2024

Just for some more context, can someone explain to me how --config <path> --config <some-other-override> differs from --config extend=<path> --config <some-other-override>?

@ofek
Copy link
Contributor Author

ofek commented Mar 15, 2024

Are you implying that specifying multiple configuration files is implicitly an extend operation? If so, then I suppose there is no feature request here but rather a documentation update.

@zanieb
Copy link
Member

zanieb commented Mar 15, 2024

I'm not sure! The configuration hierarchy is confusing and inheritance via extend is even more complicated. I'm asking for someone to try it, I guess, so we can understand what we're missing.

@ofek
Copy link
Contributor Author

ofek commented Mar 16, 2024

image

@zanieb
Copy link
Member

zanieb commented Mar 17, 2024

Oh okay, I think we can support multiple configuration files on the command line. I originally intended for it to be that way, but I think it might have been cut from scope on the initial implementation. I'd be curious to hear if it would address your use-case to support merging multiple --config file options. Also @AlexWaygood if you have any more context on why we dropped that from scope that'd be helpful, I can't remember if there were reasons it was infeasible.

@QuentinSoubeyranAqemia
Copy link

If you can have more than one config file, what happens when the extend-chain is no longer linear?

ruff --config config1.toml --config config2.toml
# config1.toml
extend = "config1_base.toml"
# config2.toml
extend = "config2_base.toml"

I guess this needs to be properly documented.


Also, as a workaround for my own use-case, I template the ruff config and dynamically write a temporary file where I replace a placehold with the actual filepath to extend-from, since the CLI itself can't handle it. It's a bit ugly but it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface configuration Related to settings and configuration
Projects
None yet
Development

No branches or pull requests

5 participants