add --field-prefix via click.option and update doc #124
Conversation
added option proposed in scikit-hep/histoprint scikit-hep#121 implement behavior of --field-prefix option uses click callback on the field option to handle this. updated docs to include help text for --field-prefix option remove type annotations One of these type annotations was actually incorrect, but I noticed that they weren't used in this file. Opted to remove instead of fix
for more information, see https://pre-commit.ci
|
Hi! Thanks a lot for this PR. Could you add a test of this feature to |
|
If github isn't notifying here, does that mean that all tests passed? If so, then I think I got it right. |
|
If you look at the output of the tests (e.g. by clicking on the green checkmark next to the latest commit). you will see that it did not actually run your test defined in the tests.py file. That is because the test function names need to start with I would expect the test to fail, since you do ask for the field You can also run |
|
Could you also remove the newlines from the option help text, so it behaves like the others, and also make the docstring of your test function follow the convention of the others? |
|
I opted to ignore positional information, but didn't include that in the help doc, so I updated that along with the requests. |
for more information, see https://pre-commit.ci
| help="String to prepend to all values indicated with --field option, " | ||
| "ignores position where this and --field options are specified.", |
There was a problem hiding this comment.
| help="String to prepend to all values indicated with --field option, " | |
| "ignores position where this and --field options are specified.", | |
| help="String to prepend to all '--field' values", |
Command line options usually do not care about order so mentioning it seems more confusing than helping.
Also need to update the README. I usually just copy the output of histoprint -h directly into it, so it is exactly the same.
|
As you can see, the tests fail now: https://github.com/scikit-hep/histoprint/actions/runs/7023198492/job/19109160857?pr=124 Aside from fixing that, you also need to add the added option to the CHANGELOG.md. Just put it as a bullet point under |
migrate prefixing logic from callback into main add test case and data update changelog
|
Realized that click did not work the way I expected, but I think keeping it all in the decorators isn't worth the complexity. So it'll be implemented as a few lines in main - which may have always been the best bet. Now running pytest locally and all tests are passing. |
added option proposed in scikit-hep/histoprint #121
add --field-prefix via click.option
adding this option modifies kwargs which causes later errors
opting to delete this key, to fix root cause in later pr
implement behavior of --field-prefix option
uses click callback on the field option to handle this.
updated docs to include help text for --field-prefix option
remove type annotations
One of these type annotations was actually incorrect, but I noticed that they weren't used in this file. Opted to remove instead of fix