rename solidity_testing_smoke to solidity_testing_sanctuary
#616
Conversation
|
| match self { | ||
| Self::SolidityTestingSanctuary => { | ||
| // This crate parses tens of thousands of Solidity files: | ||
| // It is worth spending the extra time to recompiling its dependenceis. |
There was a problem hiding this comment.
| // It is worth spending the extra time to recompiling its dependenceis. | |
| // It is worth spending the extra time to recompiling its dependencies. |
| #[clap(name = "solidity_npm_build", hide = true)] | ||
| SolidityNpmBuild, | ||
| } | ||
|
|
||
| impl RunCommand { | ||
| fn should_run_in_release_mode(&self) -> bool { | ||
| match self { |
There was a problem hiding this comment.
optional style nit - I find that 'everything is an expression' in Rust helps simplify the code in many places, this could just be
match self {
// This crate ...
Self::SolidityTestingSanctuary => true,
// These ...
... | ... => false,
}There was a problem hiding this comment.
I don't really like that return is optional in Rust. It is an important keyword in almost all programming languages that describe exactly whether the expression on the right hand side results in a value or ()/void. It is very useful when reading statements left to right to understand the context as the reader's eye is moving.
It is less of an issue in a tiny function like this, but in more complex ones, it is much more important, especially with multiple match cases or if branches.
If it is possible, I would have hoped to enforce it everywhere. Saving the few extra keystrokes is just not worth it.
There was a problem hiding this comment.
I am going to 100% disagree here. Rust is an expression-oriented language, so eliding the return statement is quite natural. I prefer it unless there are early returns, which I don't like in any case.
There was a problem hiding this comment.
Agree, I'd have to go with the prevalent code style that's used in Rust. In complex code with early return it's needed to use the return statement but for majority of cases I find that eliding the statement is more natural. I would optimize for the more common case and for familiarity/ease of onboarding other Rust devs here.
We should enable and enforce Clippy at some point and call it a day 👍
| #[clap(name = "solidity_cargo_build", hide = true)] | ||
| SolidityCargoBuild, | ||
|
|
||
| /// Runs codegen for the NAPI parser crate. |
There was a problem hiding this comment.
nit: The wording seems to imply that NAPI may somehow correspond equally to Rust but it's the "same" Rust parser crate, just with aux bindings/glue to NAPI. What do you think about clarifying this a little bit?
| /// Runs codegen for the NAPI parser crate. | |
| /// Runs codegen for the NAPI-exposed parser crate. |
No description provided.