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

feat(forge): add forge selectors cmd #5072

Merged
merged 7 commits into from
May 30, 2023
Merged

Conversation

lmanini
Copy link
Contributor

@lmanini lmanini commented May 29, 2023

Motivation

Tackles the first part of this comment on #5012.

Solution

Ported the code from forge upload-selectors over to forge selectors upload and added a warning in the former.

@Evalir Evalir requested review from mattsse and Evalir May 29, 2023 16:48
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

need to do a second pass / test, just a few comments:

@@ -28,6 +28,8 @@ pub struct UploadSelectorsArgs {
impl UploadSelectorsArgs {
/// Builds a contract and uploads the ABI to selector database
pub async fn run(self) -> eyre::Result<()> {
println!("Warning! This command is deprecated and will be removed in v1, use `forge selectors upload` instead");
Copy link
Member

Choose a reason for hiding this comment

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

i think we could make this even more flashy with some yellow color, e.g: https://github.com/foundry-rs/foundry/blob/master/cli/src/cmd/forge/script/mod.rs#L750

};

#[derive(Debug, Clone, Parser)]
pub enum SelectorsSubcommands {
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave some docs on what this is 🙏

all: bool,

#[clap(flatten)]
project_paths: ProjectPathsArgs,
Copy link
Member

Choose a reason for hiding this comment

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

Let's also document this field

Copy link
Contributor Author

@lmanini lmanini May 29, 2023

Choose a reason for hiding this comment

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

tbf I don't really know how to document this field: it doesn't look settable by the user and it's not documented over on fourbytes.rs either..
any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

these will be flattened and will use the existing documentation

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm,

pending @Evalir

can't really add tests for this since this feature requires API

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

sick, pending ci

@lmanini
Copy link
Contributor Author

lmanini commented May 29, 2023

not sure what broke the ci - ran

cargo +nightly fmt --all
cargo +nightly clippy --all --all-features -- -D warnings

before the last commit.
did I miss something?

@Evalir
Copy link
Member

Evalir commented May 29, 2023

no worries—the ci failure is a flaky test that we need to fix

@mattsse mattsse merged commit b45b519 into foundry-rs:master May 30, 2023
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.

3 participants