search

sos-os / alarm

ALARM: Another Library for Allocating and Releasing Memory
Apache License 2.0
8 stars 6 forks source link

chore(rustfmt): replace configs for new rls-rustfmt #41

Closed leshow closed 6 years ago

leshow commented 6 years ago

closes #34 The rustfmt.toml file 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.toml has 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 mentions settings.json for vscode) or leave it empty.

Thankfully, the option you have turned on workspaces_mode defaults to true, so I don't think we need to do anything.

leshow commented 6 years ago

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.

hawkw commented 6 years ago

@leshow, this looks good to me with the following additional requests (a majority of which are git style):

  1. can we merge this with the commit message "chore(rustfmt)" rather than "feat(rustfmt)", since this change doesn't add any new features that should show up in changelog?
  2. please add "Fixes #34" or "Closes #34" to the commit message, thanks!
  3. have you invoked Rustfmt with the changed config? if you have, and there were any changes to the source code, can you commit those as well? that way, future commits won't merge as many unrelated formatting changes.

Thanks!

leshow commented 6 years ago

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

hawkw commented 6 years ago

@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.

hawkw commented 6 years ago

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.

leshow commented 6 years ago

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.

https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md#blank_lines_upper_bound

hawkw commented 6 years ago

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.