dagr
dagr copied to clipboard
Various fixes for usages derived from the @arg annotation.
The fixes are described in the second commit message pasted here:
Various fixes for usages derived from the @arg annotation.
Enforce if both 'flag' and 'name' are given in the @arg annotation, the
length of 'flag' is less than or equal to the length of 'name'. It is ok
if the length of 'name' derived from the constructor-parameter/field is
less than the length of 'flag', and in this cause we'll print 'flag'
second. Examples:
@arg(flag="abc", name="a") val a: Tshould be illegal@arg(flag="abc") val a: Twill have usage "-a T, --abc=T" Previously it would not check and print the following usages for 1-2 above:- "-abc T, --a=T". Note that specifying either "-abc T" or "--a T" wouldn't work.
- "-abc T, -a=T". Note that specifying "-abc T" wouldn't work.
Format a single-character flag correctly if specified in name or the variable/field is a single-character.
@arg(name="a") val abc: Twill have usage "-a T".@arg val a: Twill have usage "-a T".
Add a regression test for the 'name' field being camel case:
@arg(name="aB") val a: Twill have usage "--aB=T".
Fix the usage for multi-character flag arguments. For example, if
@arg(flag="abc") val abcd: T then the usage will be
"--abc=T, --abcd=T"; it was previously "-abc T, --abcd=T".
Make it ok for the 'flag' and derived 'name' are the same, but only
print out one: @arg(flag="a") val a: T will be "-a T".
Updated the documentation for ArgAnnotation for all the above.
Codecov Report
Merging #188 into master will decrease coverage by
0.13%. The diff coverage is100%.
@@ Coverage Diff @@
## master #188 +/- ##
==========================================
- Coverage 91.75% 91.61% -0.14%
==========================================
Files 54 54
Lines 2159 2183 +24
Branches 196 140 -56
==========================================
+ Hits 1981 2000 +19
- Misses 178 183 +5
| Impacted Files | Coverage Δ | |
|---|---|---|
| ...r/sopt/cmdline/ClpArgumentDefinitionPrinting.scala | 98.24% <100%> (-1.76%) |
:arrow_down: |
| ...scala/dagr/sopt/cmdline/ClpReflectiveBuilder.scala | 93.68% <100%> (+1%) |
:arrow_up: |
| ...ns/src/main/scala/dagr/commons/util/TimeUtil.scala | 66.66% <0%> (-33.34%) |
:arrow_down: |
| ...scala/dagr/sopt/cmdline/CommandLineException.scala | 25% <0%> (-25%) |
:arrow_down: |
| ...src/main/scala/dagr/core/tasksystem/Pipeline.scala | 83.33% <0%> (-6.15%) |
:arrow_down: |
| ...scala/dagr/commons/reflect/ReflectiveBuilder.scala | 91.17% <0%> (-4.07%) |
:arrow_down: |
| ...mons/src/main/scala/dagr/commons/util/Logger.scala | 75% <0%> (-2.5%) |
:arrow_down: |
| ...ala/dagr/core/execsystem/TaskExecutionRunner.scala | 91.08% <0%> (-1.31%) |
:arrow_down: |
| ...rc/main/scala/dagr/core/config/Configuration.scala | 89.55% <0%> (-0.78%) |
:arrow_down: |
| ... and 27 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 4a62490...9ec712d. Read the comment docs.
Can we talk a little bit about the desired behaviour here? I read through your comment and some of the tests to make sure I understood, and I think I now do. There are a few things that I'm not sure about:
- It seems arbitrary to me to allow the field name to be shorter than flag, but not an annotated name to be shorter than flag. Why not either disallow both, or allow both and swap them when that occurs?
- I don't love allowing flag values longer than a single character if you then have to use
--abinstead of-ab. I think if we're going to support multi-character flags they should be with single-dash'd on the command line and in usage. - I really wish we had just made
flagaCharin the first place and then it'd be obvious that it was meant to be a single character. Why didn't we do that?
Mostly I would just like the end result to be that there's a very simple and intuitive transformation between what you right in a class and what the command line expects, and this adds a lot of extra rules like if the flag is multi-character, if the flag is longer than the field name, etc. that are not so intuitive to anyone else.
@tfenne
I think the solution we can agree upon is making flag a Char. Honestly, it's a breaking change, but it is the right change. I am very much for it if you don't like some of the changes I made here.
Next, I think I want a short name (or alternate name) in addition to a single character than flag, since I may want to be able to use --tldr but have the parameter in the constructor named @arg val tooLongDidntRead. Perhaps the -t flag is already in use. What do you think?
@nh13 I think your last suggestion is good. Move flag to be Char, maybe we even go so far as to support multiple alternative names, so not just short names but really any alternative you want to present?