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

fix: Fix help generation #87

Merged
merged 1 commit into from
Sep 20, 2023
Merged

fix: Fix help generation #87

merged 1 commit into from
Sep 20, 2023

Conversation

fmoura
Copy link
Contributor

@fmoura fmoura commented Sep 14, 2023

No description provided.

CHANGELOG.md Outdated
Comment on lines 20 to 23
### Fixed

- Fixed help generation for all services

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this bug wasn't in a previous release, we don't need to add a fix entry in the changelog. It is like it never existed in the first place.

#[derive(Parser, Debug)]
#[derive(Clone, Parser)]
#[command(name = "advance_runner_config")]
#[command(about = "Configuration for rollups advance-runner")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer that we remove the about section. Most CLI starts with the usage.

@@ -62,7 +62,9 @@ pub enum ConfigError {
SnapshotConfigError { source: SnapshotConfigError },
}

#[derive(Parser, Debug)]
#[derive(Clone, Parser)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some use case needed, not sure why. I was just making it standard. Will remove and re-check who really needs

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it doesn't hurt to leave it there and It is nice to be standardized. You can leave it if you want.

@fmoura fmoura requested a review from gligneul September 18, 2023 21:32
@fmoura fmoura force-pushed the fix/fix-help-gen branch 3 times, most recently from 8bae15e to 683378c Compare September 19, 2023 22:14
@fmoura fmoura added the no changelog PRs that don't require changes in changelog label Sep 20, 2023
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

The help message still needs to be clarified.

➜  offchain git:(683378c8) ./target/debug/cartesi-rollups-advance-runner --help | head
CLI configuration used to generate the DApp metadata

Usage: cartesi-rollups-advance-runner [OPTIONS] --snapshot-dir <SNAPSHOT_DIR> --snapshot-latest <SNAPSHOT_LATEST>

If it is too hard to remove the description, we could just leave something like "Configuration for advance-runner".
I didn't test the other services.

@fmoura
Copy link
Contributor Author

fmoura commented Sep 20, 2023

CLI configuration used to generate the DApp metadata

This one comes from a Comment and not an annotation. Maybe it is best to add back again the Description for every service, so we do not fallback on presenting some unwanted description that comes from a "inner" configuration that could come from an external crate

@fmoura
Copy link
Contributor Author

fmoura commented Sep 20, 2023

Correctly tested every service and they are consistent now, looking something like this

docker run -it cartesi/rollups-node:devel cartesi-rollups-state-server --help | head
Configuration for state-server

Usage: cartesi-rollups-state-server [OPTIONS]

@fmoura fmoura merged commit 9280dc4 into main Sep 20, 2023
@fmoura fmoura deleted the fix/fix-help-gen branch September 20, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs that don't require changes in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants