Merge configs from parent directories #4179
Conversation
|
Blocked by #4177 |
topecongiro
added
the
S-blocked
calebcartwright
removed
the
S-blocked
|
Still working through this (may have to circle back to it over the weekend) but could you add some tests for this too? It's one of these scenarios that'll likely need a bit of additional plumbing but this is awesome behavior that we want to have tests for. I think one approach would be setting up a subdirectory to mimic nested config files, and ignoring everything except the entry point file to properly validate the config detection and merging |
|
@calebcartwright Ive added a test doing this on lines 648-685 of rustfmt-lib/src/config.rs. Do you think we should pull it out into the tests directory? |
|
Apologies should've been more clear. It'd be great to get some integration/system tests in addition to the unit tests (not instead of) |
| cargo) | ||
| git clone --depth=1 https://github.com/rust-lang/${INTEGRATION}.git | ||
| cd ${INTEGRATION} | ||
| git clone --depth=1 https://github.com/rust-lang/${INTEGRATION}.git ${tempdir} |
There was a problem hiding this comment.
Using a temp dir so we don’t pick up rustfmt’s rustfmt.toml. This shouldn’t matter for this PR bc of the feature flag, but will long term if the PR lands.
topecongiro
left a comment
topecongiro
left a comment
There was a problem hiding this comment.
Thank you for the PR! Looks good to me overall.
| } | ||
| } | ||
|
|
||
| macro_rules! update_config { |
There was a problem hiding this comment.
Do we still need this macro?
There was a problem hiding this comment.
Yep, see the usage in fill_from_parsed_config on line 164.
Currently, `rustfmt` stops building its configuration after one
configuration file is found. However, users may expect that `rustfmt`
includes configuration options inhereted by configuration files in
parent directories of the directory `rustfmt` is run in.
The motivation for this commit is for a use case involving files that
should be ignored by `rustfmt`. Consider the directory structure
```
a
|-- rustfmt.toml
|-- b
|-- rustfmt.toml
|-- main.rs
```
Suppose `a/rustfmt.toml` has the option `ignore = ["b/main.rs"]`. A user
may expect that running `rusfmt` in either directory `a/` or `b/` will
ignore `main.rs`. Today, though, if `rustfmt` is run in `b/`, `main.rs`
will be formatted because only `b/rustfmt.toml` will be used for
configuration.
Although motivated by the use case for ignored files, this change sets
up `rustfmt` to incrementally populate all configuration options from
parent config files. The priority of population is closest config file
to most transient config file.
To avoid churn of the existing implementation for ignoring files,
populating the ignored files config option with multiple config files is
done by computing a join. Given config files "outer" and "inner", where
"inner" is of higher priority (nested in the directory of) "outer", we
merge their `ignore` configurations by finding the ignore files
specified in "outer" that are present in the "inner" directory, and
appending this to the files ignored by "inner".
This means that printing an `ignore` configuration may not be entirely
accurate, as it could be missing files that are not in the current
directory specified by transient configurations. These files are out of
the scope the `rustfmt` instance's operation, though, so for practical
purposes I don't think this matters.
Closes rust-lang#3881
topecongiro
left a comment
topecongiro
left a comment
There was a problem hiding this comment.
LGTM, thank you for making updates!
calebcartwright
added
1x-backport:pending
4 participants
Currently,
rustfmtstops building its configuration after oneconfiguration file is found. However, users may expect that
rustfmtincludes configuration options inhereted by configuration files in
parent directories of the directory
rustfmtis run in.The motivation for this commit is for a use case involving files that
should be ignored by
rustfmt. Consider the directory structureSuppose
a/rustfmt.tomlhas the optionignore = ["b/main.rs"]. A usermay expect that running
rusfmtin either directorya/orb/willignore
main.rs. Today, though, ifrustfmtis run inb/,main.rswill be formatted because only
b/rustfmt.tomlwill be used forconfiguration.
Although motivated by the use case for ignored files, this change sets
up
rustfmtto incrementally populate all configuration options fromparent config files. The priority of population is closest config file
to most transient config file.
To avoid churn of the existing implementation for ignoring files,
populating the ignored files config option with multiple config files is
done by computing a join. Given config files "outer" and "inner", where
"inner" is of higher priority (nested in the directory of) "outer", we
merge their
ignoreconfigurations by finding the ignore filesspecified in "outer" that are present in the "inner" directory, and
appending this to the files ignored by "inner".
This means that printing an
ignoreconfiguration may not be entirelyaccurate, as it could be missing files that are not in the current
directory specified by transient configurations. These files are out of
the scope the
rustfmtinstance's operation, though, so for practicalpurposes I don't think this matters.
Closes #3881