-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
Initial CLI package implementation w/ tests and updated examples #1897
Conversation
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.
Great work putting this together!
I would have preferred the API surface details go through the RFC process, but I know we had some internal disagreement over whether that was already covered by the CLI format RFC, so we can just discuss the API details here, and make sure we get all/most interested parties satisfied with them before we merge this.
A few general comments:
I did notice some style issues, but for the most part I didn't call them out individually. Please take a look at the style guide that @Theodus helpfully started in #1894 - it hasn't been merged yet, but it's mostly settled and the remaining points of discussion should be settled quite soon. In particular, please look over your indentation and your method signatures to be sure they match the style guide.
Another general comment is that I see a lot of public fields on types, and I'd encourage you to think about which if any of them could be made private. In terms of keeping APIs stable, it's usually a good idea to err on the side of exposing less, since we can always move something from private to public (or expose it with a getter method) if there is a need, but going from public to private is an API breakage. I expect this package to get used in a lot of different projects, and considering the impressive amount of effort that went into it, I'd really prefer that we don't break it going forward if we don't have to.
If there are fields with info that you think really needs to be public in the API, I'd encourage you to think about the option of making the fields private, and providing a public getter method. This will give more flexibility in the future if we need to change the internal implementation of the fields, but we can still comply with the same public contract.
packages/cli/command_spec.pony
Outdated
( BoolType | ||
| StringType | ||
| I64Type | ||
| F64Type) |
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.
These primitives don't seem to be part of the public API used for specifying and extracting CLI args.
If that's true, could these types be made private?
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.
Sure, I will double check. I think at one point I had the user passing these as type selectors but then I hid that behind the methods.
packages/cli/command_spec.pony
Outdated
let name: String | ||
let descr: String | ||
let options: col.Map[String, OptionSpec] = options.create() | ||
var help_name: String = "xxx" // TODO: make this nice. |
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.
Is this TODO supposed to still be here?
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 will remove it before merging. I introduced it with some last minute help improvements, so I'll double check things and remove it.
packages/cli/command_parser.pony
Outdated
else | ||
// TODO: Ponyc should know we've covered all match cases above | ||
SyntaxError(arg, | ||
"Pony shouldn't allow this: unknown value type " + typ.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.
This will be fixed when my "exhaustive match" PR (#1891) gets merged, which will hopefully happen today. So you might want to subscribe to that PR, and update this PR by removing the unnecessary "else" clause here if my PR is merged first.
packages/cli/command_parser.pony
Outdated
| let se: SyntaxError => se | ||
else | ||
// TODO: Ponyc should know we've covered all match cases above | ||
SyntaxError(arg, "Pony shouldn't allow this") |
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.
This will be fixed when my "exhaustive match" PR (#1891) gets merged, which will hopefully happen today. So you might want to subscribe to that PR, and update this PR by removing the unnecessary "else" clause here if my PR is merged first.
Also, you to have 4 spaces of indentation increase here, when it should be 2, so if you aren't able to remove this clause before this PR is merged, then please decrease the indent.
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.
OK.
packages/cli/command_parser.pony
Outdated
| None => SyntaxError(short_string(c), "unknown short option") | ||
else | ||
// TODO: Ponyc should know we've covered all match cases above | ||
return SyntaxError(token, "Pony shouldn't allow this") |
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.
This will be fixed when my "exhaustive match" PR (#1891) gets merged, which will hopefully happen today. So you might want to subscribe to that PR, and update this PR by removing the unnecessary "else" clause here if my PR is merged first.
However, in this particular case, you could avoid the problem entirely by converting your | None
clause into an else
clause, if you like (then you could remove the existing else
clause).
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 am very excited for #1891!
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.
It just got merged, BTW - so it is on master, and it will be in the next release.
packages/cli/command_help.pony
Outdated
primitive Columns | ||
fun indent(n: USize): String => | ||
let spaces = " " | ||
spaces.substring(0, n.isize()) |
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.
Can n
ever be larger than the number of spaces in spaces
? What is the intended result in that case?
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.
Possibly. This output support is intended as a temporary solution until we can add something more complete and polished.
packages/cli/command_help.pony
Outdated
This interface and two classes allow the help output to go to a general Writer | ||
that can be implemented for string creation or to an OutStream. Or other | ||
targets. This could be polished up and moved into a more central package. | ||
""" |
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'd prefer that this interface and two classes be made private, since they aren't really a part of the core features that need to be provided by this package, and they clutter the namespace.
Also, this docstring in this position doesn't attach to the interface and classes - it will either attach to the package or get discarded (I don't remember which one) - neither is what you want, so this docstring should either be turned into a comment, or moved into the interface and class defs.
packages/cli/command_help.pony
Outdated
_os.write(data) | ||
|
||
|
||
primitive Columns |
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.
This seems like it's just an internal helper, in which case the type should be made private.
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.
Sure.
packages/cli/command_help.pony
Outdated
(CommandHelp | SyntaxError) | ||
=> | ||
let ch = CommandHelp._create(None, cs) | ||
_parse(cs, ch, argv) |
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.
Since both of these methods require a CommandSpec box
as the first argument, it seems like they could be moved to be a fun box
on the CommandSpec
type.
Then we could eliminate the need for the Help
type, or at least make it private if you wanted to keep the _parse
method here.
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 did want to keep the help support separate from the core CommandSpec. Making Help private would work for me.
I understand your concern about the lack of API RFC. I did want to have my implementation working and reviewed before having a discussion around the API spec. Once we are happy with this PR, I could put together a CLI API RFC that documents what we decide here. I will review the diff against the style guide. It's amazing how the deviations stand out once you see them as diffs in GitHub. The indentations problems were just reformatting mistakes, and the signatures are probably just my lack of knowledge of #1894 which I will now study. The public fields are there intentionally as a style I used based on Clean Code's Objects vs. Data Structures, and are especially nice with immutability language support. I've used it in Java and it's a pretty common style in Golang. Making the fields private and adding public accessors doesn't reduce the public API lock-in all that much as it still has specific "properties" by name and type. You do gain some ability to cons up some properties behind the function calls but at the cost of easy of use. If Pony supported Uniform Access Principle more completely here we could change from field access to method calls anytime in the future. (so this is a long-winded explanation, but I am not really tied to this approach I took). I am also happy to make more of the types private. |
This looks fantastic! Perhaps at some point the parser could support reading and writing config files that specify user defined defaults to commands and arguments. This way a user of a cli using program could:
Admittedly supporting config files is not trivial, but I do like the functionality it provides users for the programs that provide them. My previous experience and exposure to such functionality comes from Python's ConfigArgParse package. The way it allows you to specify argument values as Environment variables is also something I found quite useful. |
cd83a38
to
9098f86
Compare
This package now compiles and tests pass as of master with the exhaustive match feature. There are 3 small workarounds for #1898 which we can remove when that is fixed. I have updated the syntax to I think match the #1775 style guide. Let me know if I missed or misunderstood anything. I will review the other suggestions about public fields and types in the next day or two. One other example of exposing immutable fields publicly is in I also want to think about type extensibility so that users can add additional option and arg types like String lists, or parsed Strings like dates and URLs. Thanks @patternspandemic . I had thought about multiple token sources, hence the distinct parse() step. But I hadn't thought all the way through to how that would work with config files. I like your suggestions and I'll look at that Python package. (this cli package does support env supplied option values, they are just implicitly named vars based on the app and option name.) |
c557226
to
21d82ae
Compare
I have updated the code to address most of the comments. There are a couple of features I'd like to figure out and a few cleanup details that I'm not sure are OK as is. I'm taking notes in a GoogleDoc here: So feel free to comment there as well as here. |
I have been stuck tinkering with a solution for the "cleaner post-parse options & args access" feature that I mention in my doc above. The current approach with querying for results by string and then getting an untyped Value back is pretty tedious and weak. I'd like to have something where the end result is correctly typed variables or struct fields that get populated by the parser. In other languages, I might have used macros or reflection or other metalanguage features. In Golang, and probably C/C++, pointers to or pass-by-ref of the output variables would be the simplest way. I've been trying to get a callback to work here by passing a lambda to the spec for each option and arg to be output. This is when I ran into #1928 and got stuck. All my approaches either result in that compiler exception or a Maybe there's a better way to do this in Pony that I am missing. Or maybe my use of lambdas is off. Any suggestions? |
It turns out Lambdas don't really help here as they can't write back to values in their capture scope—they just make (shadowing) copies. Maybe some other form of callbacks... |
@cquinn have you considered Promises? |
@cquinn - if you're having trouble with API design, let's just get a simple API for this PR, get it merged, then do an RFC process so we can collaborate on the API design of the next iteration. |
@SeanTAllen I hadn't looked at Promises, but had been iterating into something similar. Promises seem a little heavier than needed for this, though, where all we really need is an intra-actor mutable value wrapper or box. Like:
@jemc yes, you are probably right. Better good now than tinkering forever. I'll review what is already here and tested and polish up any small things remaining. |
71bf4d4
to
5cea1ec
Compare
I think the API now is ready for review. I've privatized all of the fields, tuned all the caps I could, and updated all the examples that used options. The gups_opt example had a nice pattern of using a Config class to hold all the program's config settings, so I expanded on that in many of the examples. |
examples/gups_basic/main.pony
Outdated
| let se: SyntaxError => | ||
env.out.print(se.string()) | ||
env.exitcode(1) | ||
error |
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.
Using extra indentation in match
case bodies doesn't match the standard library style - these should be at the same indentation level as the let
.
This comment also applies to some other match
blocks in this PR.
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.
Oops, my mistake. Copy & paste indentation problem. Fixing now.
packages/cli/command_help.pony
Outdated
Writer that can be implemented for string creation or to an OutStream. Or | ||
other targets. This could be polished up and moved into a more central | ||
package. | ||
""" |
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 still don't really think that Writer
, StringWriter
, and OutWriter
belong as public types provided by this package. As you recognize in the docstring, they're not intrinsically related to CLI concerns.
Also, for types/concepts as fundamental as these I'd really like to see some robust RFC discussion about the best approach to provide a consistent API layer for this concern. It really is a big enough discussion that it deserves to be separate from this one, and I can already think of some concerns I have about the design as it's presented here (for example, it doesn't account for writev
needs, and the many-append
approach of StringWriter
is not particularly optimized).
Instead of slipping them in here and migrating them to a different package later, I'd prefer to see them removed from here and added later, once the design has been worked out.
For the time being, I'd suggest refactoring to use the Writer
type from the buffered
package, which already exists and serves a similar purpose to that of your StringWriter
. For the public methods that accept your Writer
type as an argument, I suggest something like this:
class box CommandHelp
// ...
fun help_string(): String =>
let w: buffered.Writer = buffered.Writer
_write_help(w)
let size = w.size()
let string = recover trn String(size) end
for bytes in s.done().values() do string.append(bytes) end
consume string
fun print_help(os: OutStream) =>
let w: buffered.Writer = buffered.Writer
_write_help(w)
os.writev(w.done())
fun _write_help(w: buffered.Writer) =>
print_usage(w)
if _spec.descr().size() > 0 then
w.write("\n")
w.write(_spec.descr() + "\n")
end
let options = all_options()
if options.size() > 0 then
w.write("\nOptions:\n")
print_options(w, options)
end
if _spec.commands().size() > 0 then
w.write("\nCommands:\n")
print_commands(w)
end
let args = _spec.args()
if args.size() > 0 then
w.write("\nArgs:\n")
print_args(w, args)
end
// ...
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.
That's a great idea. I'm on it. I don't remember buffered.Writer being that applicable to my case.
… EnvVars to primitive.
0324149
to
06f7172
Compare
packages/cli/command_spec.pony
Outdated
| None => (typ', false, true) | ||
| let d: Value => (typ', d, false) | ||
else | ||
// Ponyc limitation: else can't happen, but segfaults without 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.
Just checking: is this still true? If so, is there already a ticket for this particular exhaustive match bug?
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.
Yes, this still happens, and I meant to create a smaller repro case. I'll try to put that together, but I'll create a ticket in the meantime track it and refer to this PR as a case. Here is the issue: #1975
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.
There's one more small style point that I've noticed, but other than that, this looks ready to merge.
I think there are probably some things we could still do to improve the API, such as maybe using type parameters for OptionSpec
and the like (OptionSpec[I64](...)
instead of OptionSpec.i64(...)
), but as discussed above let's work asynchronously to improve the API. This PR is a great place to start.
Thank you for putting all the work in to make this happen - I know this one was a doozy!
packages/cli/command_help.pony
Outdated
primitive Help | ||
|
||
fun general(cs: CommandSpec box): CommandHelp | ||
=> |
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.
Style-wise, the preceding line seems short enough that this double-arrow could be at the end of that line instead of having its own line here, without breaking the 80 columns rule.
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.
OK, no problem.
I had thought about using generics while I was in the midst of refactoring, but didn't want to go off on a long tangent. I have one more commit to add which makes the Value and ValueType classes/primitives private. Should have that in the next couple hours. |
One question I have @jemc: the OptionSpec and ArgSpec classes need to expose some things like defaults and types to the CommandParser. But these are not really meant for public consumption and some return private types. Should I make all these methods private to the package as a general rule? |
@cquinn - Yeah, I think as a general rule, methods and types should be private to the package unless they are intended to be used as part of the public API that the package exposes. |
@cquinn - Have you seen this question in my earlier comment? #1897 (comment) |
Implementation of https://github.com/ponylang/rfcs/blob/master/text/0038-cli-format.md.
All rules and syntaxes are supported, and a help command and option are supported.
One small example is included, but the tests are pretty thorough as far as specs and command line combinations go.
I still need to deprecate the options package (I'm not sure how to do that). And update existing examples and the tutorial.
As a follow up, I also want to add support for list types, (lists of String, etc.), and a 'no' prefix to negate bool options.
I'm open to suggestions, especially regarding Pony usage, and how the value types are handled.