Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Various fixes for usages derived from the @arg annotation. #188

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nh13
Copy link
Member

@nh13 nh13 commented Aug 17, 2016

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 nh13 force-pushed the nh_arg_annotation_fixups branch 2 times, most recently from 90fd1e2 to 8bd62d0 Compare August 17, 2016 04:21
@codecov-io
Copy link

codecov-io commented Aug 17, 2016

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%) ⬇️
...scala/dagr/sopt/cmdline/ClpReflectiveBuilder.scala 93.68% <100%> (+1%) ⬆️
...ns/src/main/scala/dagr/commons/util/TimeUtil.scala 66.66% <0%> (-33.34%) ⬇️
...scala/dagr/sopt/cmdline/CommandLineException.scala 25% <0%> (-25%) ⬇️
...src/main/scala/dagr/core/tasksystem/Pipeline.scala 83.33% <0%> (-6.15%) ⬇️
...scala/dagr/commons/reflect/ReflectiveBuilder.scala 91.17% <0%> (-4.07%) ⬇️
...mons/src/main/scala/dagr/commons/util/Logger.scala 75% <0%> (-2.5%) ⬇️
...ala/dagr/core/execsystem/TaskExecutionRunner.scala 91.08% <0%> (-1.31%) ⬇️
...rc/main/scala/dagr/core/config/Configuration.scala 89.55% <0%> (-0.78%) ⬇️
... 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.

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:
1. "-abc T, --a=T". Note that specifying either "-abc T" or "--a T"
wouldn't work.
2. "-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.
@tfenne
Copy link
Member

tfenne commented Aug 27, 2016

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.

@nh13
Copy link
Member Author

nh13 commented Aug 29, 2016

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

@tfenne
Copy link
Member

tfenne commented Dec 23, 2016

@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 tfenne assigned nh13 and unassigned tfenne Dec 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants