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

Display 1-dimensional table in benchie show (closes #16) #24

Merged
merged 2 commits into from
May 6, 2022

Conversation

mwidmoser
Copy link
Contributor

No description provided.

CliCommand::Show { row, col, metric } => match (row, col, metric) {
(Some(row), Some(col), Some(metric)) => show_2d_table(row, col, metric),
(Some(row), _, Some(metric)) => show_1d_table(row, metric),
_ => show(),
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: A way to make this a bit more easy to understand, you could also split CliCommand::Show in CliCommand::Show, CliComnmand::Show1dTable and CliCommand::Show2dTable. Therefore only specific permutations of row, col and metric are allowed by construction. But it is OK as you implemented it, because the argument parsing should limit the allowed permutations anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed I also considered this kind of solution but wasn’t sure if it’s better or not atm 🤔 .

@ChristianMoesl
Copy link
Contributor

Ah one additional comment: It's good to split unit tests in very small function, such that you can test them independently. And the name of the unit test function is the most important one. Because it should describe, what constrain you are testing. E.g. 1d_tables_should_not_allow_a_col_argument. Therefore anyone knows immediately what your are testing there.

@mwidmoser
Copy link
Contributor Author

Done.

@mwidmoser mwidmoser merged commit 6208a86 into main May 6, 2022
@mwidmoser mwidmoser deleted the show-1d-table branch May 6, 2022 18:15
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.

None yet

2 participants