Support formatting config via rustfmt.toml #331
Conversation
Are you using a very recent nightly? I think you'll need one from the last few days. |
|
This is great, thank you! I was thinking of tackling this this week, and lo! There is a PR for it - great timing :-) |
src/config.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl Deref for FmtConfig { |
There was a problem hiding this comment.
I don't like this pattern of using Deref for newtypes, see https://github.com/rust-unofficial/patterns/blob/master/anti_patterns/deref.md for why
| impl FmtConfig { | ||
| /// Look for `.rustmt.toml` or `rustfmt.toml` in `path`, falling back | ||
| /// to the default config if neither exist | ||
| pub fn from(path: &Path) -> FmtConfig { |
There was a problem hiding this comment.
Could we use the code in Rustfmt for this? Even if it means changing the API in Rustfmt a bit, I'd prefer that, rather than duplicating code, if we don't have to.
There was a problem hiding this comment.
I took a crack at this: rust-lang/rustfmt#1613
src/config.rs
Outdated
| @@ -155,3 +158,50 @@ create_config! { | |||
| cfg_test: bool, true, false, "build cfg(test) code"; | |||
| unstable_features: bool, false, false, "enable unstable features"; | |||
| } | |||
|
|
|||
| /// A rustfmt config (typically specified via rustfmt.toml) | |||
| pub struct FmtConfig(rustfmt::config::Config); | |||
There was a problem hiding this comment.
Could you document why you are using a newtype, rather than just the rustfmt config directly please
* Add docs * Remove deref conversion to rustfmt's config
|
Attempted to address your newtype comments. For the rustfmt one, I think it requires merging the changes we added to master down to the libsyntax branch (not super familiar with it, when I tried merging myself there were some test failures I didn't know how to fix). |
I've rebased the libsyntax branch, you should be good to go now. |
Hm, don't see my changes in there, did you forget to push? Or am I missing something... |
Whoops, pushed to the wrong repo. Should be fixed now |
|
Thanks! pushed an update. |
|
This looks great now, thank you! |
Although formatting is behind an unstable flag, I find it usable other than the lack of support for project rustfmt.toml, one of the bullets in #3 . If this isn’t the approach you’d like to take to configuring rustfmt (i.e. you want to pass it via
DocumentFormattingParamsor some other configuration scheme) feel free to reject.Also, the recently added reformatting unit test fails for me even before this change with a rustfmt parsing error.