CLI package update 1 #2019

cquinn · 2017-07-08T06:55:48Z

Features added:

string_seq option and arg value type for repeated strings collected into a sequence.
Command fullname() to make it easier to match on command name.
Checking commands are leaves so that partial commands return syntax errors.

Other changes:

Tests for the above.
_ValueType is now a trait instead of a union. This makes it easier to delegate type specific work to one place, and makes value type more easily extended.
A few other cleanups, like the removal of redundant box modifiers on methods.

SeanTAllen · 2017-07-08T08:35:57Z

This should have a CHANGELOG entry (or entries) for features added.

Can you do a short writeup of the changes and how they should be in release notes when this goes out? I don't think anyone but you is capabale of writing up those notes @cquinn. It would be good to get them recorded now.

jemc · 2017-07-08T14:06:31Z

packages/cli/_test.pony

-    else
-        return // error was expected
+      let cs = CommandSpec.leaf("min imal", "")
+      h.fail("expected error on bad command name" + cs.name())


You probably want to append : to your string literal here.

Otherwise the cs.name() bit is going to get printed squished up against the word "name" from the string literal when printed.

jemc · 2017-07-08T14:11:30Z

packages/cli/command_spec.pony

@@ -325,22 +343,66 @@ class val ArgSpec
    _name + "[" + _typ.string() + "]" +
      if not _required then "(=" + _default.string() + ")" else "" end

-type _Value is (Bool | String | I64 | F64)
+class _StringSeq is ReadSeq[String]


This looks like a persistent data structure holding a sequence of strings. Have you thought about using Vec[String] from the collections/persistent package instead of rolling your own here?

Let me give that a look. I started with this simple wrapper class to get a Stringable and a String sequence of some kind. But a persistent structure might make more sense given the way it has to be updated during parse.

I think I still need the wrapper class, but could replace the Array[String] with a Vec[String] to avoid the array copying.

jemc · 2017-07-08T14:14:30Z

packages/cli/command_spec.pony

+    ss.append(ss1.strings)
+    strings = ss
+
+  fun string(): String =>


If you're aiming to fulfill the Stringable interface, this should be returning a String iso^, which should be possible if you remove the trn from your recover statement below (so that it recovers to iso instead).

jemc · 2017-07-08T14:15:42Z

packages/cli/command_spec.pony

+      if str.size() > 1 then str.append(",") end
+      str.append(s)
+    end
+    str.append("]")


For each of these append calls where you're appending a single-character string, you could be pushing just the single character (as a U8) instead. For example: str.push(']')

cquinn · 2017-07-08T18:49:53Z

@SeanTAllen I can certainly write up CHANGELOG entries. Can you point me to an example I can refer to?

SeanTAllen · 2017-07-08T19:45:29Z

Check out the existing CHANGELOG entries. I'd say this PR is multiple entries.

cquinn · 2017-07-10T00:44:46Z

@SeanTAllen I guess I/we never wrote up changelog entries for the CLI package at all. I can take a stab at that too. Should I just add an edit to CHANGELOG.md in this PR?

SeanTAllen · 2017-07-10T06:33:23Z

Yes to CHANGELOG entries being part of this PR

cquinn · 2017-07-11T05:02:42Z

I've added a CHANGELOG entry to the unreleased section for the whole CLI implementation in this and the initial PR.

jemc · 2017-07-11T15:10:57Z

@cquinn - I don't fully understand the semantics of the string_seq argument type yet, and that made me realize it doesn't seem to be documented anywhere. Is there somewhere in the cli package where we can document how each argument type works, including string_seq?

cquinn · 2017-07-11T15:52:13Z

@jemc yes, I will take some time now to document the types and the package as a whole, and add that to this PR.

…lname. Checking commands are leaves.

…ests. Fixed bug where unknown short options were missed.

cquinn · 2017-07-16T20:22:32Z

@jemc @SeanTAllen I have added more doc comments, and a big CHANELOG entry. I'm not sure if the CHANGELOG entry is too much and maybe I should pare it down.

jemc · 2017-07-17T02:29:41Z

Thanks for the extra docs, it's great!

Maybe I'm being obtuse, but I still don't really understand what the CLI args for a "string seq" are supposed to look like, though... 😕

SeanTAllen · 2017-07-17T12:02:47Z

@cquinn changelog looks good to me.

cquinn · 2017-07-17T19:03:25Z

@jemc I've added more doc comments. The idea behind a string_seq is that it is a repeated option or arg that produces a sequence of strings after parsing. It can be used, for example, to allow multiple filenames to be passed to a tool that read them.

The examples/commandline/main.pony uses a string_seq to echo all of the command line args.

jemc · 2017-07-17T20:28:09Z

If I have an Option of a string_seq type, how does that work with other args and options - where does the string sequence associated with the option stop?

cquinn · 2017-07-17T21:18:48Z

With Options, the option prefix has to be used each time, like:
--file=f1 --file=f2 --file=f3 and the results are collected into one string_seq.
With Args, there is no way to indicate termination, so a string_seq Arg should be the last arg and will consume all remaining command line tokens.

Maybe I need to include this doc somewhere as a comment.

jemc · 2017-07-17T21:25:58Z

Got it. It makes sense now.

Yes, I'd definitely like to see that in the documentation, since I don't think I'm the only one who will have that question 👍.

…onylang#2049

cquinn · 2017-07-18T22:54:48Z

@jemc How does it look now?

jemc · 2017-07-18T23:26:08Z

Great, thanks.

jemc reviewed Jul 8, 2017

View reviewed changes

cquinn force-pushed the cli_package_update1 branch 2 times, most recently from 503db79 to 1aa3933 Compare July 11, 2017 03:10

cquinn added 6 commits July 15, 2017 16:30

CLI package update: string_seq option and arg value type. Command ful…

92a9378

…lname. Checking commands are leaves.

Fixed bad formatting

3d53ec0

Small fixes for PR comments

30d8da3

Using persistent Vec instead of Array for string_seq collection

c02cfa8

Added changelist entry for the two PRs for RFC-0038 implementation

481f808

Simplified example and added to new cli package doc. More doc, more t…

67c1403

…ests. Fixed bug where unknown short options were missed.

cquinn force-pushed the cli_package_update1 branch from 8e10426 to 67c1403 Compare July 15, 2017 23:30

More doc comments to spec class methods.

d5492d9

cquinn added 2 commits July 17, 2017 21:38

More package doc, with specifics on string_seq.

b3b6687

Removed workaround for issue ponylang#1975 that's now fixed as of PR p…

85c911d

…onylang#2049

jemc merged commit 3fb8eec into ponylang:master Jul 18, 2017

cquinn deleted the cli_package_update1 branch July 18, 2017 23:29

cquinn restored the cli_package_update1 branch February 6, 2019 02:43

cquinn deleted the cli_package_update1 branch February 25, 2019 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI package update 1 #2019

CLI package update 1 #2019

cquinn commented Jul 8, 2017 •

edited

Loading

SeanTAllen commented Jul 8, 2017

jemc Jul 8, 2017

cquinn Jul 8, 2017

jemc Jul 8, 2017

cquinn Jul 8, 2017

cquinn Jul 8, 2017

cquinn Jul 11, 2017

jemc Jul 8, 2017

cquinn Jul 8, 2017

jemc Jul 8, 2017

cquinn Jul 8, 2017

cquinn commented Jul 8, 2017

SeanTAllen commented Jul 8, 2017

cquinn commented Jul 10, 2017

SeanTAllen commented Jul 10, 2017

cquinn commented Jul 11, 2017

jemc commented Jul 11, 2017

cquinn commented Jul 11, 2017

cquinn commented Jul 16, 2017

jemc commented Jul 17, 2017

SeanTAllen commented Jul 17, 2017

cquinn commented Jul 17, 2017 •

edited

Loading

jemc commented Jul 17, 2017

cquinn commented Jul 17, 2017 •

edited

Loading

jemc commented Jul 17, 2017

cquinn commented Jul 18, 2017

jemc commented Jul 18, 2017

CLI package update 1 #2019

CLI package update 1 #2019

Conversation

cquinn commented Jul 8, 2017 • edited Loading

SeanTAllen commented Jul 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cquinn commented Jul 8, 2017

SeanTAllen commented Jul 8, 2017

cquinn commented Jul 10, 2017

SeanTAllen commented Jul 10, 2017

cquinn commented Jul 11, 2017

jemc commented Jul 11, 2017

cquinn commented Jul 11, 2017

cquinn commented Jul 16, 2017

jemc commented Jul 17, 2017

SeanTAllen commented Jul 17, 2017

cquinn commented Jul 17, 2017 • edited Loading

jemc commented Jul 17, 2017

cquinn commented Jul 17, 2017 • edited Loading

jemc commented Jul 17, 2017

cquinn commented Jul 18, 2017

jemc commented Jul 18, 2017

cquinn commented Jul 8, 2017 •

edited

Loading

cquinn commented Jul 17, 2017 •

edited

Loading

cquinn commented Jul 17, 2017 •

edited

Loading