Skip to content

Cargo fmt#1550

Merged
dani-garcia merged 6 commits into
dani-garcia:masterfrom
RealOrangeOne:fmt
Apr 15, 2021
Merged

Cargo fmt#1550
dani-garcia merged 6 commits into
dani-garcia:masterfrom
RealOrangeOne:fmt

Conversation

@RealOrangeOne
Copy link
Copy Markdown
Contributor

Run cargo fmt on the codebase, and add it to CI

@dani-garcia
Copy link
Copy Markdown
Owner

I think we should try to tweak rustfmt's style a bit, for example, this change:

let steps: i64 = if CONFIG.authenticator_disable_time_drift() { 0 } else { 1 };

let steps: i64 = if CONFIG.authenticator_disable_time_drift() {
    0
} else {
    1
};

Looks worse afterwards.

Then sometimes it decides to not split lines at all, like this:

let json = json!({"page_content": "admin/login", "version": VERSION, "error": msg, "urlpath": CONFIG.domain_path()});
let json =
    json!({"page_content": "admin/login", "version": VERSION, "error": msg, "urlpath": CONFIG.domain_path()});

Which I assume is because of the macro, but i'd rather that change not be done.

There are others too, particularly with long strings as function parameters, where it moves the string to a new line and it barely reduces the width, in those cases it might be better to split the string and escape the newline with \

I don't know if those things are configurable in rustfmt, if they aren't I'll just have to deal with it 😄

@RealOrangeOne
Copy link
Copy Markdown
Contributor Author

@dani-garcia I've re-applied to updated master, and manually reviewed and re-flowed a couple lines manually.

@BlackDex
Copy link
Copy Markdown
Collaborator

BlackDex commented Apr 4, 2021

I did some testing, and i think i have a nice config. The only thing missing is a new feature so that chains are not being put on one line as we now have a lot of those separated on a new-line, and using fmt now will compress those to one line.

In v2.x they have this, and i see that there is a PR open for a backport of this feature: rust-lang/rustfmt#4782 , which i would really like to see merged, and then we could use that to have it more inline with how our code looks right now.

These is btw my tested settings

version = "Two"
edition = "2018"
max_width = 225
newline_style = "Unix"
use_small_heuristics = "Off"
struct_lit_single_line = false
overflow_delimited_expr = true

@RealOrangeOne
Copy link
Copy Markdown
Contributor Author

max_width = 225
Seems excessive?

I'll try running with that config, see what the diff looks like.

@BlackDex
Copy link
Copy Markdown
Collaborator

BlackDex commented Apr 4, 2021

max_width = 225
Seems excessive?

I'll try running with that config, see what the diff looks like.

I checked with wc -L which files had long lines, and that seems what we are comfortable with now.

@RealOrangeOne
Copy link
Copy Markdown
Contributor Author

Turns out there's already a rustfmt.toml file in the repo, with a length of 120. Given that's a far more sane number, and what the rest of this PR was formatted with, I left it. 225 was making lines far longer than we'd probably want.

@dani-garcia dani-garcia merged commit 8756c5c into dani-garcia:master Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants