chore(rustfmt): replace configs for new rls-rustfmt #41
Conversation
|
I took a look at the rustfmt.toml from hawkw/sos-kernel and put some of the options you have there in it. If you're happy with this configuration it may be worth it to have the same file in both projects. |
|
@leshow, this looks good to me with the following additional requests (a majority of which are git style):
Thanks! |
leshow
changed the title
|
Quick note, the old line limit was defaulted to 80, the new rustfmt line limit appears to be 100. That's the reason for a few of the changes to the src files. Let me know if you'd like to alter the char width limit and I'll re-run rustfmt |
|
@leshow I have a reasonably strong personal preference for 80 column lines, so I'd prefer to see lines wrapped at the 80th column. However, I'm not so attached to this that I'm unwilling to negotiate if a majority of contributors prefer 100 column lines. 🙂 I noticed that this diff also removed whitespace between trait items (e.g. function defs), and modified the licence header comments in some files. I'd prefer not to make those changes if rustfmt still has appropriate config options for this. |
|
Sorry for being so finicky about code style; if you'd prefer, I can also go ahead and merge this PR, and make additional rustfmt changes later. |
|
No problem at all. I don't have a strong opinion about char widths so long as it's legible. 80 is fine with me. All I can see w/ license headers is it removing a blank character after a comment in one file, is that what you mean? As for the new line between trait and function defs (I hope we're talking about the same thing). I've gone through their configuration doc a few times and the closest thing I can find is a blank line upper and lower bound, however the number is global so it won't be restricted to space in between traits. The config options have been drastically reduced since about a year ago when I last looked at rustfmt's options. |
Yeah, that's why the current config file raises so many warnings. :/ If those things aren't easily configurable, I'm going to go ahead and approve this PR now, and maybe take a closer look at the Rustfmt config options later. |
|
|
||
| fn borrow<'a, T>( | ||
| &'a self, | ||
| ) -> Result<Borrowed<'a, T, Self::Allocator>, AllocErr>; |
There was a problem hiding this comment.
Ugh, I hate the way this looks; it would be really nice if rustfmt would still allow us to set the "Visual" indent style just for function declarations, like we used to be able to. I'm considering just setting the visual indent style, but that can lead to things like closures being indented weirdly.
There was a problem hiding this comment.
Hahah, the new rustfmt goes with block style for almost everything. I used to have an opinion like that about the positioning of the 'where' keyword in the new rfc style, but honestly you just get used to it.
| reorder_imports = true | ||
|
|
||
| # Reorder lists of names in import statements alphabetically. | ||
| # Default: false | ||
| reorder_imported_names = true |
There was a problem hiding this comment.
What's the relationship between this configuration and reorder_imports that was added in this PR? Just out of curiosity.
There was a problem hiding this comment.
They have 3 options for reordering imports as I understand it. reorder_imports is the base option that needs to be true and handles the use a; use b; import ordering, reorder_imported_names reorders inuse foo::{a, b, c} and the 'groups' one reorders blocks of imports that are local vs 3rd party, grouping them together and sorting them in each group.
There was a problem hiding this comment.
Ah, I see, thanks! The naming of those configurations are...not terribly clear.
| skip_children = false | ||
|
|
||
| # Ideal width of each line. | ||
| # Default: 80 | ||
| max_width = 80 |
closes #34
The
rustfmt.tomlfile had mostly defaults, with a few exceptions. Some of those options don't exist anymore but I tried to find the exceptions and turn them on.rls.tomlhas been deprecated (https://github.com/rust-lang-nursery/rls). We're meant to either make what seems like an editor specific project file (the main readme mentionssettings.jsonfor vscode) or leave it empty.Thankfully, the option you have turned on
workspaces_modedefaults to true, so I don't think we need to do anything.