-
Notifications
You must be signed in to change notification settings - Fork 0
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 more metadata about a CLP and its arguments. #19
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19 +/- ##
==========================================
- Coverage 92.72% 91.97% -0.76%
==========================================
Files 15 15
Lines 756 785 +29
Branches 97 95 -2
==========================================
+ Hits 701 722 +21
- Misses 55 63 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get very far. I really don't think the values belong in the Metadata classes. I don't think leveraging those to enable printing of the command line is a good way to go. Why not just have the parser implement that functionality?
) extends MarkDownDescription | ||
) extends MarkDownDescription { | ||
/** Returns a command line string representation of the arguments. If user-specified values are set on the args, | ||
* it will use those, otherwise the defaults if present, otherwise it the type of the argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last part after the comma doesn't read well. Also, why would it output the type if there is no user set value or default value? I would say they should be omitted if no value is present.
/** Returns a command line string representation of the arguments. If user-specified values are set on the args, | ||
* it will use those, otherwise the defaults if present, otherwise it the type of the argument. | ||
* | ||
* @param withDefaults true to include args with defaults, otherwise those with user-set values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the doc is wrong or I'm misunderstanding. I think you always use the user set values, and if withDefaults
is true, you also include options that have default values but no user set value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right to be confused. If I rename this includeArgsWithDefaults
:
- If
false
, then only include arguments who have values set by the user. - If
true
, also include arguments with default values.
) extends MarkDownDescription | ||
description: String, | ||
isSpecial: Boolean, | ||
userValue: Option[Seq[String]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this. These classes are supposed to be metadata about the class/args. Stuffing values into them is weird and makes then seem more like the internal implementation classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't think
userValue
is correct, as it could also be the default value. - The use case I have is inspecting the class built from command line parsing, so I want to know the value. For example, I have built a tool from the arguments on the command line, now I want to know for a given argument for that tool, does it have a value and if so, what is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tfenne I am coming back to this (to close out PRs), and we should discuss your comment here
/** Returns a command line string representation of the arguments. If user-specified values are set on the args, | ||
* it will use those, otherwise the defaults if present, otherwise it the type of the argument. | ||
* | ||
* @param withDefaults true to include args with defaults, otherwise those with user-set values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right to be confused. If I rename this includeArgsWithDefaults
:
- If
false
, then only include arguments who have values set by the user. - If
true
, also include arguments with default values.
) extends MarkDownDescription | ||
description: String, | ||
isSpecial: Boolean, | ||
userValue: Option[Seq[String]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't think
userValue
is correct, as it could also be the default value. - The use case I have is inspecting the class built from command line parsing, so I want to know the value. For example, I have built a tool from the arguments on the command line, now I want to know for a given argument for that tool, does it have a value and if so, what is it?
@tfenne here's the rabbit hole I went down.