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

add support for numbers alignment in table output (#1202) #1236

Merged
merged 5 commits into from
Apr 22, 2021

Conversation

cmanning09
Copy link
Contributor

previously numbers alignment was hardcoded to 'right'. This change adds
the ability to set a configuration value 'numbers.align' in the
formatting section of the config to alter the format of the table output.

previously numbers alignment was hardcoded to 'right'. This change adds
the ability to set a configuration value 'numbers.align' in the
formatting section of the config to alter the format of the table output.

Signed-off-by: cmanning09 <cmanning09@users.noreply.github.com>
@danielmitterdorfer danielmitterdorfer added this to the 2.2.0 milestone Apr 12, 2021
@danielmitterdorfer danielmitterdorfer added :Reporting Command line reporting enhancement Improves the status quo labels Apr 12, 2021
@danielmitterdorfer
Copy link
Member

Thanks for your interest in Rally and your PR! We'll start the review process shortly.

@elasticmachine test this please

@DJRickyB
Copy link
Contributor

Hi! Thank you for this. For the race subcommand things work as I would expect, but I wanted to note that when I use Rally's compare subcommand I get a failure, and it prints an error if the configuration is not defined:

[ERROR] Cannot compare. No value for mandatory configuration: section='reporting', key='numbers.align'

From the log:

Traceback (most recent call last):
  File "/Users/rick.boyd/dev/rally/esrally/config.py", line 178, in opts
    return self._opts[self._k(scope, section, key)]
KeyError: (<Scope.application: 1>, 'reporting', 'numbers.align')

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/rick.boyd/dev/rally/esrally/rally.py", line 774, in dispatch_sub_command
    reporter.compare(cfg, args.baseline, args.contender)
  File "/Users/rick.boyd/dev/rally/esrally/reporter.py", line 48, in compare
    ComparisonReporter(cfg).report(
  File "/Users/rick.boyd/dev/rally/esrally/reporter.py", line 315, in __init__
    self.numbers_align = config.opts("reporting", "numbers.align")
  File "/Users/rick.boyd/dev/rally/esrally/config.py", line 183, in opts
    raise exceptions.ConfigError(f"No value for mandatory configuration: section='{section}', key='{key}'")
esrally.exceptions.ConfigError: No value for mandatory configuration: section='reporting', key='numbers.align'

@cmanning09
Copy link
Contributor Author

Good catch. I have addressed the issue you raised. Please let me know if there is anything else.

@DJRickyB
Copy link
Contributor

wonderful! thank you @cmanning09. This code functions as I'd expect, and as you indicated in the initial commit it addresses #1202 nicely.

I have one more ask/iteration of you: this functionality should be documented. please consider whether it will be supported similarly to the reporting.format key, in that it's only documented as the CLI --report-format flag (in which case more code would be required), or if users should always specify it in the [reporting] section of their local .ini. Note that the documentation for that section states This section defines how metrics are stored. and so would need to be updated for the new meaning.

Thank you!

@cmanning09
Copy link
Contributor Author

I addressed your concern by adding support through CLI flags for both race and compare subcommands. Is there anything else?

@DJRickyB
Copy link
Contributor

Beautiful! I've tested it and it works as expected. Please add the documentation and then I think we're all set. (Should be in docs/command_line_reference.rst)

Copy link
Contributor

@DJRickyB DJRickyB left a comment

Choose a reason for hiding this comment

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

Thank you!

@DJRickyB
Copy link
Contributor

@elasticmachine run tests

@DJRickyB
Copy link
Contributor

@cmanning09, just as an update: I am having trouble getting this branch to pass our internal CI. I don't believe this is due to your code, so please bear with me. Once tests pass cleanly I will merge this one in

@DJRickyB
Copy link
Contributor

@elasticmachine update branch

@DJRickyB
Copy link
Contributor

@elasticmachine run tests

@DJRickyB DJRickyB merged commit 2ecb9a3 into elastic:master Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo :Reporting Command line reporting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants