-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
IMHO we should add a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked over 1/4 of these changes and am happy with them.
One thing I noticed is that the auto generated weight files are reformatted, too. This will break the CI check whenever those are updated (by the bot). I think we should either run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might cause some issues for PR authors, but it needs to be done anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the first 60 files. Overall I think rustfmt does a very good job and there's not much to fix but I found some issues with weird formatting around: multi-line strings and really long expression lines (e.g. matching on the result of a function call). For multi-line strings maybe the format_strings
option should help, for other cases introducing some temporary variables helps with making the code more readable. If we could somehow coordinate people to actually look at the diffs then we could probably catch all regressions, we only need 10 people to look at 100 files each :)
You can enable it with `--features runtime-benchmarks`." | ||
.into()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks weird. Wouldn't format_string = true
help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes format_strings would help but it messes up things like client/cli/src/commands/utils.rs:
println!(
"Network ID/version: {}\n \
Public key (hex): {}\n \
Public key (SS58): {}\n \
Account ID: {}\n \
SS58 Address: {}",
So our choice is to manually indent multiline strings (keeping control) or to add rustfmt::skips where we need more control. On the whole I think doing multi-line strings manually means less surprises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's about 5 places in the codebase where it messes up (mostly snippits of json). Personally I'd not format strings but given there's only 5 places where it causes harm 🤷 ...
crate::mock::new_test_ext(), | ||
crate::mock::Test, | ||
); | ||
impl_benchmark_test_suite!(Template, crate::mock::new_test_ext(), crate::mock::Test,); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl_benchmark_test_suite!(Template, crate::mock::new_test_ext(), crate::mock::Test,); | |
impl_benchmark_test_suite!(Template, crate::mock::new_test_ext(), crate::mock::Test); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this will happen in a bunch of other places. Maybe should be reported as an issue to rustfmt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so we can in general remove those commas as we find them and it won't put them back.
Maybe we have a separate follow-up PR that nurfs those extra commas at the end of macros?
] | ||
.iter() | ||
{ | ||
for block_type in [ | ||
BlockType::RandomTransfersKeepAlive, | ||
BlockType::RandomTransfersReaping, | ||
BlockType::Noop, | ||
].iter() { | ||
for database_type in [BenchDataBaseType::RocksDb, BenchDataBaseType::ParityDb].iter() { | ||
import_benchmarks.push((profile, size.clone(), block_type.clone(), database_type)); | ||
] | ||
.iter() | ||
{ | ||
for database_type in | ||
[BenchDataBaseType::RocksDb, BenchDataBaseType::ParityDb].iter() | ||
{ | ||
import_benchmarks.push(( | ||
profile, | ||
size.clone(), | ||
block_type.clone(), | ||
database_type, | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe introducing some variables for the arrays would make this code less ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we want to keep this PR as small as possible in terms of manual code changes. Beautification should be a followup PR.
Err("Benchmarking wasn't enabled when building the node. \ | ||
You can enable it with `--features runtime-benchmarks`.".into()) | ||
} | ||
} | ||
You can enable it with `--features runtime-benchmarks`." | ||
.into()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the earlier issue with formatting multi-line strings.
Some(Subcommand::TryRuntime) => Err("TryRuntime wasn't enabled when building the node. \ | ||
You can enable it with `--features try-runtime`." | ||
.into()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's leaving untouched the leading spaces. If we manually fix the indenting of these lines then rustfmt will retain them.
node_runtime::WASM_BINARY.expect( | ||
"Development wasm binary is not available. \ | ||
Testing is only supported with the flag disabled.", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong.
"Development wasm binary is not available. \ | ||
Testing is only supported with the flag disabled.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong.
let header = match executor_call::<NeverNativeValue, fn() -> _>( | ||
env, | ||
"BlockBuilder_finalize_block", | ||
&[0u8;0], | ||
&[0u8; 0], | ||
true, | ||
None, | ||
).0.unwrap() { | ||
) | ||
.0 | ||
.unwrap() | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a temp var for the result of executor_call
would probably make this less weird.
"Development wasm binary is not available. This means the client is \ | ||
built with `SKIP_WASM_BUILD` flag and it is only usable for \ | ||
production chains. Please rebuild with the flag disabled.") | ||
production chains. Please rebuild with the flag disabled.", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong.
@@ -233,6 +233,14 @@ cargo-deny: | |||
# FIXME: Temorarily allow to fail. | |||
allow_failure: true | |||
|
|||
cargo-fmt: | |||
stage: test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a debug step, just to avoid running everything else, move it to the previous stage
stage: test | |
stage: .pre |
just do not forget to revert this at the end :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ty, but that works ;)
mem, | ||
rx, | ||
) | ||
.unwrap_err(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again here if we pull these strings out into vars they will format fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's best not to do any manual changes in this PR otherwise it will make it harder to deal with conflicts on existing PRs.
bot merge |
Waiting for commit status. |
No description provided.