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

Format the codebase via rustfmt #263

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

primeos-work
Copy link
Member

@primeos-work primeos-work commented Jul 11, 2023

The first commit is the result of cargo fmt and the second commit adds a CI check to ensure that we won't run into this "problem" again. This PR is quite overdue (I delayed it to avoid unnecessary conflicts with other active/open PRs (especially #132)).

The first commit also contains a small manual change as running cargo fmt resulted in the following regression:

$ cargo check -r
    Checking butido v0.4.0
error: unnecessary braces around method argument
   --> src/commands/endpoint.rs:405:20
    |
405 |             .chain({ data.values().flat_map(|hm| hm.keys()).map(|s| s.deref()) })
    |                    ^^                                                         ^^
    |
note: the lint level is defined here
   --> src/main.rs:32:5
    |
32  |     unused,
    |     ^^^^^^
    = note: `#[deny(unused_braces)]` implied by `#[deny(unused)]`
help: remove these braces
    |
405 -             .chain({ data.values().flat_map(|hm| hm.keys()).map(|s| s.deref()) })
405 +             .chain(data.values().flat_map(|hm| hm.keys()).map(|s| s.deref()))
    |

error: could not compile `butido` (bin "butido") due to previous error

We could also consider overriding some rustfmt settings (https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=) to reduce the size of the diff / avoid somewhat "unnecessary" changes.

This is just the result of running `cargo fmt`. Unfortunately, quite a
few changes are necessary as `rustfmt` wasn't used for quite a while.
We'll add a CI check to enforce that the code is properly formatted.

Signed-off-by: Michael Weiss <[email protected]>
To enforce a consistent formatting of the code based on the official
`rustfmt` code formatting tool (and custom settings in `rustfmt.toml`).

Signed-off-by: Michael Weiss <[email protected]>
@@ -1264,7 +1265,8 @@ fn arg_older_than_date(about: &str) -> Arg {
months, month, M -- defined as 30.44 days
years, year, y -- defined as 365.25 days

"#)
"#,
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't ideal... :(

Comment on lines -350 to +389
writeln!(outlock, "For Package: {p} {v}",
writeln!(
outlock,
"For Package: {p} {v}",
p = mkgreen(&db_package.name),
v = mkgreen(&db_package.version))?;
v = mkgreen(&db_package.version)
)?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This destroys the manual formatting but IMO it's fine

Comment on lines 491 to 493
let hdrs = crate::commands::util::mk_header(vec![
"Submit",
"Job",
"Time",
"Host",
"Ok?",
"Package",
"Version",
"Distro",
"Submit", "Job", "Time", "Host", "Ok?", "Package", "Version", "Distro",
]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll produce another "large" diff if we extend the vector but I guess that's acceptable (at least it's consistent (based on the length)).

Comment on lines +319 to +340
d.config
.exposed_ports
.map(|hm| {
let s = hm
.iter()
.map(|(k, v_hm)| {
format!(
"{:ind$}{k}:\n{hm}",
"",
ind = 8,
k = k,
hm = v_hm
.iter()
.map(|(k, v)| format!(
"{:ind$}{k}: {v}",
"",
ind = 12,
k = k,
v = v
))
.collect::<Vec<_>>()
.join("\n")
Copy link
Member Author

@primeos-work primeos-work Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch... That might deserve a rewrite :D (s. also the other parts in this file)

Comment on lines 66 to 69
.values(&new_submit)

// required because if we re-use the staging store, we do not create a new UUID but re-use the old one
.on_conflict_do_nothing()

.execute(conn)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ideal but it should still be clear that the comment belongs to the next line.

Comment on lines -46 to +53
LogItem::Line(l) => {
LogItem::Line(l) => {
let s = std::str::from_utf8(l).unwrap_or("ERROR UTF8 ENCODING");
writeln!(f, "[{i}] Line('{s}')")?
},
LogItem::Progress(u) => writeln!(f, "[{i}] Progress({u})")?,
}
LogItem::Progress(u) => writeln!(f, "[{i}] Progress({u})")?,
LogItem::CurrentPhase(s) => writeln!(f, "[{i}] Phase({s})")?,
LogItem::State(Ok(_)) => writeln!(f, "[{i}] State::OK")?,
LogItem::State(Err(_)) => writeln!(f, "[{i}] State::Err")?,
LogItem::State(Ok(_)) => writeln!(f, "[{i}] State::OK")?,
LogItem::State(Err(_)) => writeln!(f, "[{i}] State::Err")?,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of such alignments so this is fine with me :)
(The alignments look pretty but I don't like their impact on diffs + they're annoying to maintain, unless it's automated by the IDE/editor/formatter)

@christophprokop christophprokop added this pull request to the merge queue Jul 18, 2023
Merged via the queue into science-computing:master with commit b210621 Jul 18, 2023
@primeos-work primeos-work mentioned this pull request Jul 26, 2023
@primeos-work primeos-work mentioned this pull request Jan 9, 2023
11 tasks
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.

2 participants