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

Migrate initial work from fuel-core #1

Merged
merged 23 commits into from Jun 6, 2022
Merged

Migrate initial work from fuel-core #1

merged 23 commits into from Jun 6, 2022

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Mar 21, 2022

Migrated from FuelLabs/fuel-core#204

As per debugger meeting on 2022-03-15, the content here is migrated to a new repo.

After this has been merged and published:

  • Do related work on fuel-core so the debugger can be used
  • Release fuel-debugger to crates.io

@Dentosal Dentosal marked this pull request as ready for review March 25, 2022 09:36
README.md Outdated Show resolved Hide resolved
@Dentosal Dentosal force-pushed the initial-work branch 2 times, most recently from 4df8a46 to bf2a987 Compare March 26, 2022 03:13
Cargo.toml Outdated Show resolved Hide resolved
client/Cargo.toml Outdated Show resolved Hide resolved
client/src/names.rs Outdated Show resolved Hide resolved
Dentosal and others added 3 commits March 26, 2022 16:23
client/Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>
client/Cargo.toml Outdated Show resolved Hide resolved
client/src/main.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,14 @@
[package]
Copy link
Member

@Voxelot Voxelot Mar 28, 2022

Choose a reason for hiding this comment

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

We should use a flat cargo workspace rather than a nested crate. Otherwise publishing, lockfiles and caching etc will be more complicated.

client/src/lib.rs Outdated Show resolved Hide resolved
};
}

async fn parse_and_run_command(client: &mut Client, text: &str) -> io::Result<()> {
Copy link
Member

@Voxelot Voxelot Mar 28, 2022

Choose a reason for hiding this comment

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

The async-friendly shellfish crate might be a nicer alternative to the manual parsing here: https://crates.io/crates/shellfish

Copy link
Member Author

Choose a reason for hiding this comment

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

Shellfish looks suitable. A simple google search didn't find me anything else than doing it with clap which wasn't really good.

Choose a reason for hiding this comment

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

Oh, I kinda like clap. But using some lib to parse arguments seems really like the way to go

Copy link
Member Author

Choose a reason for hiding this comment

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

Clap is really good for command line argument parsing, but doesn't allow customizing things enough to be nice for interactive CLI apps.

Copy link

@vlopes11 vlopes11 left a comment

Choose a reason for hiding this comment

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

1st round of comments

client/src/lib.rs Outdated Show resolved Hide resolved
client/src/lib.rs Outdated Show resolved Hide resolved
client/src/lib.rs Outdated Show resolved Hide resolved
client/src/lib.rs Outdated Show resolved Hide resolved
client/src/main.rs Outdated Show resolved Hide resolved
client/src/main.rs Outdated Show resolved Hide resolved
};
}

async fn parse_and_run_command(client: &mut Client, text: &str) -> io::Result<()> {

Choose a reason for hiding this comment

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

Oh, I kinda like clap. But using some lib to parse arguments seems really like the way to go

client/src/names.rs Outdated Show resolved Hide resolved
@Dentosal
Copy link
Member Author

Blocked by FuelLabs/fuel-core#260

@Voxelot
Copy link
Member

Voxelot commented Apr 19, 2022

Would it be possible to write integration tests for this CLI against fuel-core?

@Dentosal Dentosal requested review from vlopes11 and Voxelot May 30, 2022 17:14
Copy link

@vlopes11 vlopes11 left a comment

Choose a reason for hiding this comment

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

Looks good overall; small optional style nit

return Err(Box::new(ArgError::Invalid));
};

let contract = if let Some(contract_id) = contract_id {
Copy link

Choose a reason for hiding this comment

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

Consider using combinators instead

uses: baptiste0928/cargo-install@v1
with:
crate: fuel-core
version: "0.8"
Copy link
Member

@Voxelot Voxelot Jun 2, 2022

Choose a reason for hiding this comment

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

[nit] consider moving "0.8" to a github env var

@Dentosal Dentosal merged commit c6382be into master Jun 6, 2022
@Dentosal Dentosal deleted the initial-work branch June 6, 2022 16:37
@Hamz0i
Copy link

Hamz0i commented Jul 15, 2023

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants