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

Make -r argument to 'report' required and improve usage message #155

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

wash-amzn
Copy link
Contributor

Before, the --help message didn't indicate multiple -r arguments could be supplied to report

[ec2-user@ip-10-0-0-129 ~]$ ./aperf report --help
Generate an HTML report based on the data collected

Usage: aperf report [OPTIONS]

Options:
  -r, --run <RUN>    Run data to be visualized. Can be a directory or a tarball
  -n, --name <NAME>  Report name
  -v, --verbose...   Show debug messages. Use -vv for more verbose messages
  -h, --help         Print help
  -V, --version      Print version

and moreover, you didn't even have to specify any. Doing so would get you an empty report (not very useful).

[ec2-user@ip-10-0-0-129 aperf]$ ./target/debug/aperf report -n foo
[2024-04-08T14:54:05Z INFO  aperf_lib::report] Creating APerf report...
[2024-04-08T14:54:05Z INFO  aperf_lib::report] Generating foo.tar.gz

[ec2-user@ip-10-0-0-129 aperf]$ cat foo/data/js/runs.js
runs_raw = []

With this change, the usage message shows the ... and running without any -r arguments results in an error

[ec2-user@ip-10-0-0-129 aperf]$ ./target/debug/aperf report --help
Generate an HTML report based on the data collected

Usage: aperf report [OPTIONS] --run <RUN>...

Options:
  -r, --run <RUN>...  Run data to be visualized. Can be a directory or a tarball
  -n, --name <NAME>   Report name
  -v, --verbose...    Show debug messages. Use -vv for more verbose messages
  -h, --help          Print help
  -V, --version       Print version

[ec2-user@ip-10-0-0-129 aperf]$ ./target/debug/aperf report -n foo
error: the following required arguments were not provided:
  --run <RUN>...

Usage: aperf report --run <RUN>... --name <NAME>

For more information, try '--help'.

@wash-amzn wash-amzn requested a review from a team as a code owner April 8, 2024 15:21
@wash-amzn wash-amzn force-pushed the main branch 2 times, most recently from cdb8cf6 to b7bc237 Compare April 9, 2024 13:39
architecture: [X64, ARM64]
distribution: [Ubuntu, AL2]
architecture: [ARM64]
distribution: [Amazon Linux 2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intent here not to test X64 and Ubuntu combination?

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'm testing stuff in my personal account's fork of this, but unfortunately github wants to keep automatically updating this PR. Sorry about that, you'll have to ignore the noise for the moment.

@wash-amzn wash-amzn force-pushed the main branch 2 times, most recently from f898cb9 to c9b73ca Compare April 9, 2024 15:17
@wash-amzn wash-amzn force-pushed the main branch 2 times, most recently from 543eae0 to 8d9a3a3 Compare April 9, 2024 15:56
@wash-amzn wash-amzn merged commit 9e17ee1 into aws:main Apr 9, 2024
8 checks passed
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