switch to non-recursive mode by default#3938
Conversation
39b65f2 to
fe5f668
Compare
|
Thanks for the feedback @scampi! |
topecongiro
left a comment
There was a problem hiding this comment.
Thank you for the PR!
Configurations.md
Outdated
| } | ||
| ``` | ||
|
|
||
| ## `recursive` |
There was a problem hiding this comment.
We should consider recursive as one of the internal options. We do not want users to add this to the configuration by themselves.
There was a problem hiding this comment.
Will do 👍, though out of curiosity, why should this be a CLI only flag for users?
One downside I see with exposing recursive as a config option is that recursive = false in the config file would functionally be ignored when using cargo fmt since cargo fmt will always pass --recursive flag to rustfmt which will always take precedence over the config file value (which could be confusing for users).
There was a problem hiding this comment.
The options in the configuration file are meant to be persistent, whereas recursive is more of a one-shot configuration, which best suits as a CLI flag.
There was a problem hiding this comment.
Makes sense to me! It wouldn't surprise me if there is a future request to support recursive as a config option since skip_children was available, but there's clearly several good reasons why recursive should be internal/cli-only.
Side note, while making recursive internal I noticed that internal options added to the config file seem to just be silently ignored. Do I have that correct?
If they are silently ignored:
Is that the desired behavior or would it beneficial to emit a warning ("Warning: Unknown configuration option ..)? Would that potential change be tackled (perhaps it already is) as part of #3873 or should I open a new issue?
There was a problem hiding this comment.
Side note, while making recursive internal I noticed that internal options added to the config file seem to just be silently ignored. Do I have that correct?
Yes.
Is that the desired behavior or would it beneficial to emit a warning ("Warning: Unknown configuration option ..)?
We should make it to a hard error rather than silently ignoring it.
Would that potential change be tackled (perhaps it already is) as part of #3873 or should I open a new issue?
This should be fixed by #3873.
topecongiro
left a comment
There was a problem hiding this comment.
My apologies for the late review. LGTM, thank you for the update!
Resolves #3587 and refs #3887
This PR updates the default mode of
rustfmtto not format submods in external files, with an accompanying new CLI arg (--recursive)/config option to enable recursive submod formatting mode (which was the default behavior of rustfmt 1.x).cargo fmtbehavior remains unchanged, ascargo fmtwill automatically pass the--recursiveflag torustfmt.I went ahead and marked the new
recursiveconfig option as stable because it is relatively simple. Also because it would be pretty weird IMO if bothrecursiveandskip_childrenwere only available on nightly, especially withskip_childrennow deprecated (that would make it impossible to format sub mods on stable).Lastly, I did a bit of refactoring in cargo fmt to consolidate/simplify the adding of args to
rustfmt(for example the transformation of--message-formatto--emit ..for rustfmt,--check, etc.) which had been discussed on some earlier threads.