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.

Sign up for GitHub

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

rename solidity_testing_smoke to solidity_testing_sanctuary #616

Merged
merged 2 commits into from
Oct 23, 2023
Merged

rename solidity_testing_smoke to solidity_testing_sanctuary #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3d0aae
rename `solidity_testing_smoke` to `solidity_testing_sanctuary`
OmarTawfik Oct 23, 2023
332743c
PR Comments
OmarTawfik Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ members = [
"crates/solidity/outputs/npm/crate",
"crates/solidity/outputs/npm/package",
"crates/solidity/outputs/spec",
"crates/solidity/testing/smoke",
"crates/solidity/testing/sanctuary",
"crates/solidity/testing/snapshots",
"crates/solidity/testing/utils",
]
Expand Down Expand Up @@ -58,7 +58,7 @@ solidity_npm_build = { path = "crates/solidity/outputs/npm/build" }
solidity_npm_crate = { path = "crates/solidity/outputs/npm/crate" }
solidity_npm_package = { path = "crates/solidity/outputs/npm/package" }
solidity_spec = { path = "crates/solidity/outputs/spec" }
solidity_testing_smoke = { path = "crates/solidity/testing/smoke" }
solidity_testing_sanctuary = { path = "crates/solidity/testing/sanctuary" }
solidity_testing_snapshots = { path = "crates/solidity/testing/snapshots" }
solidity_testing_utils = { path = "crates/solidity/testing/utils" }
#
Expand Down
4 changes: 2 additions & 2 deletions crates/infra/cli/generated/infra.zsh-completions
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 23 additions & 7 deletions crates/infra/cli/src/commands/run/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,42 @@ enum RunCommand {
* User-facing:
*
*/
/// Run the public 'slang_solidity' crate shipped to Cargo users.
/// Runs the public 'slang_solidity' crate shipped to Cargo users.
#[clap(name = "slang_solidity")]
SlangSolidity,

/// Run smoke tests of the Solidity parser against source files from the Sanctuary repositories.
#[clap(name = "solidity_testing_smoke")]
SolidityTestingSmoke,
/// Runs the Solidity parser against source files from the Sanctuary repositories.
#[clap(name = "solidity_testing_sanctuary")]
SolidityTestingSanctuary,

/*
*
* Hidden (for automation only):
*
*/
/// Runs codegen for the Rust parser crate.
#[clap(name = "solidity_cargo_build", hide = true)]
SolidityCargoBuild,

/// Runs codegen for the NAPI-exposed parser crate.
#[clap(name = "solidity_npm_build", hide = true)]
SolidityNpmBuild,
}

impl RunCommand {
fn should_run_in_release_mode(&self) -> bool {
match self {
Copy link
Contributor

Choose a reason for hiding this comment

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

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,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Self::SolidityTestingSanctuary => {
// This crate parses tens of thousands of Solidity files:
// It is worth spending the extra time to recompiling its dependencies.
return true;
}
Self::SlangSolidity | Self::SolidityCargoBuild | Self::SolidityNpmBuild => {
// These run during local development. Just build in debug mode.
return false;
}
};
}
}

impl RunController {
pub fn execute(&self) -> Result<()> {
let crate_name = self.command.clap_name();
Expand All @@ -48,7 +64,7 @@ impl RunController {

let mut command = Command::new("cargo").arg("run");

if self.command == RunCommand::SolidityTestingSmoke {
if self.command.should_run_in_release_mode() {
command = command.flag("--release");
}

Expand Down
4 changes: 4 additions & 0 deletions crates/solidity/testing/sanctuary/.cargo/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[build]
# This crate builds and runs in release mode.
# Let's use a separate target-dir "$THIS_CRATE/target" to avoid invalidating the workspace-level cache.
target-dir = "./target"
2 changes: 1 addition & 1 deletion crates/solidity/testing/smoke/Cargo.toml → crates/solidity/testing/sanctuary/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[package]
name = "solidity_testing_smoke"
name = "solidity_testing_sanctuary"
version.workspace = true
rust-version.workspace = true
edition.workspace = true
Expand Down
2 changes: 1 addition & 1 deletion ...olidity/testing/smoke/src/datasets/git.rs → ...ity/testing/sanctuary/src/datasets/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ impl Dataset for GitDataset {
}

fn prepare(&self) -> Result<Vec<PathBuf>> {
let dataset_dir = CargoWorkspace::locate_source_crate("solidity_testing_smoke")?
let dataset_dir = CargoWorkspace::locate_source_crate("solidity_testing_sanctuary")?
.join("target/git-datasets")
.join(self.name);

Expand Down
0 ...olidity/testing/smoke/src/datasets/mod.rs → ...ity/testing/sanctuary/src/datasets/mod.rs
View file
Open in desktop
File renamed without changes.
0 crates/solidity/testing/smoke/src/main.rs → ...es/solidity/testing/sanctuary/src/main.rs
View file
Open in desktop
File renamed without changes.
0 ...s/solidity/testing/smoke/src/reporting.rs → ...lidity/testing/sanctuary/src/reporting.rs
View file
Open in desktop
File renamed without changes.
Loading