dagr icon indicating copy to clipboard operation
dagr copied to clipboard

Various fixes for usages derived from the @arg annotation.

Open nh13 opened this issue 9 years ago • 4 comments

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:

  1. @arg(flag="abc", name="a") val a: T should be illegal
  2. @arg(flag="abc") val a: T will have usage "-a T, --abc=T" Previously it would not check and print the following usages for 1-2 above:
  3. "-abc T, --a=T". Note that specifying either "-abc T" or "--a T" wouldn't work.
  4. "-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: T will have usage "-a T".
  • @arg val a: T will have usage "-a T".

Add a regression test for the 'name' field being camel case:

  • @arg(name="aB") val a: T will 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.

nh13 avatar Aug 17 '16 04:08 nh13

Codecov Report

Merging #188 into master will decrease coverage by 0.13%. The diff coverage is 100%.

Impacted file tree graph

@@            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 data Powered by Codecov. Last update 4a62490...9ec712d. Read the comment docs.

codecov-io avatar Aug 17 '16 04:08 codecov-io

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:

  1. 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?
  2. I don't love allowing flag values longer than a single character if you then have to use --ab instead 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.
  3. I really wish we had just made flag a Char in 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 avatar Aug 27 '16 16:08 tfenne

@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 avatar Aug 29 '16 00:08 nh13

@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?

tfenne avatar Dec 23 '16 20:12 tfenne