Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cargo fmt #1550

Merged
merged 6 commits into from
Apr 15, 2021
Merged

Cargo fmt #1550

merged 6 commits into from
Apr 15, 2021

Conversation

RealOrangeOne
Copy link
Contributor

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

@dani-garcia
Copy link
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
Contributor Author

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

@BlackDex
Copy link
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
Contributor Author

max_width = 225
Seems excessive?

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

@BlackDex
Copy link
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
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