Issue #108: Shortcuts for boolean arguments

[alexshadley]

In response to this issue. This adds a feature where a shortcut can be used for boolean arguments of functions. For example the function

def foo(bar=False):

can be invoked as foo -b to set bar to True. While developing this, I came across a couple concerns:

• In detecting whether or not arguments matched the pattern of a bool shortcut, I decided that python's regex library was the best way to solve this problem. Is adding re to imports ok?
• If a naming collision occurs (see final assert of testBoolShortcutParsing) the program raises a FireError describing the ambiguity. Is this a proper manner to use this error in?
• Currently, if a shortcut doesn't match any function argument, it is parsed as-is. Should something else be done in this scenario?

[alexshadley]

Ok, evidently it took some time for me to get my style straight. Let me know if you have any questions/ things for me to fix!

[dbieber]

tl;dr I think we should support single-character flags (like your PR does), but not restrict them to bool-only values.

---

I was previously toying with the idea of allowing users to specify a flag (bool or otherwise) that is a prefix of exactly one valid parameter, but ultimately decided against that idea. Here is my thinking there (stronger points in bold):

Pros:

• This enables single-character flags
• And is additionally even more general, handling the case where multiple params start with the same letter.

Cons:

• It enables people to accidentally pass args without knowing precisely what arg they're passing
• Backwards compatibility for Fire CLIs
  • For example, if there's a Fire CLI with param send_message, a user might make a script using that CLI using the flag --send. If the CLI author later wants to add a new param send_attachment, then the user's script becomes ambiguous and breaks.
• Backwards compatibility for Fire itself

---

Your proposal differs in two significant ways: single-character-only instead of arbitrary prefixes, and bool-only instead of any type.

Single-character only still gets the most important Pro, which is supporting single-character flags.
Single-character only still has the most important Con too, which is that a single-character flag may be invalidated later.
Single-character only doesn't have as much of the final Con (backwards compatibility for Fire) since it promises a strict subset of what arbitrary-prefix does to Fire's users.
Similarly bool-only promises a strict subset of what any-type does.
However, bool-only deviates from one of Fire's current properties which is that you can pass any value (literal/container of literals) for any argument.

So my thought is that we should support single-character flags, but not restrict them to bool-only.

[dbieber]

Thanks for putting this together!

I'm looking forward to getting this feature into Fire.

Separately but relatedly I also think we should support single-hyphen flags. This takes some caution since negative numbers are valid literals, but shouldn't be a problem to do.

[dbieber]

Let's refactor arg.startswith('--') or re.match('^-[a-z]$', arg) into a function _IsFlag so that we don't have to worry about keeping the regexes in sync. And then let's use that function both here and at 792 as part of handling any type (not just bools) with single-character args.

I think the way we should organize this is (pseudocode):

def _IsFlag: return _IsSingleCharFlag or _IsMultiCharFlag
def _IsSingleCharFlag: ...
def _IsMultiCharFlag: ...

line 792:

if _IsFlag(argument):
  if _IsSingleCharFlag(argument):
    # your logic from 838 for determining the keyword
  else:
    # the logic from 796 for determining the keyword
  # continue from from 797 as is...

---

Then (either in this PR or in a subsequent change) I'm a proponent of modifying _IsMultiCharFlag to work even if argument only starts with -[a-zA-Z] instead of requiring two hyphens.

[dbieber]

Instead of always setting to True, let's use the same logic as above for supporting 'contains_equals', checking whether the next arg is a flag, and setting the value accordingly.

I think it's best if we merge the elif that you've added with the if above at 792 since so much of the logic will be shared. I try to clarify this idea in the above comment, but let me know if you want me to clarify further.

[dbieber]

I think you meant '--alpha'

[alexshadley]

Yup! Just pushed a fix

[dbieber]

Oh, looks like you're actively working on this (excellent!), I'll hold off on looking through until you're settled.
If you're put off by having to go to travis to check the lint errors (you get there by clicking the red x next to your commit) I can clean those up on my end instead, just lmk.

[dbieber]

we also need to handle the len(potential_args) == 0 case.

[alexshadley]

Should we also throw an error in this case? My original assumption here was that if the flag doesn't match, we would assume it was an value for another flag. Now that I think about it though this could cause confusing behavior.

[dbieber]

Yeah I think we should throw an error in this case.
If the argument looks like a flag (_IsFlag), it shouldn't be acceptable as a value.
[And I think we'll also broaden what _IsFlag accepts too in a separate change.]

[alexshadley]

Ok that sounds good to me, I'll add a branch and a test case for it

[alexshadley]

Sorry about the mess I'm making with these formatting commits by the way! I'll start running pylint locally so we don't have to look at this.

[dbieber]

Don't worry about the mess -- we'll squash and merge anyway.
And awesome work, glad you were able to satisfy the linter :)
I'll reply to your q now

[alexshadley]

Alright, I've run out of time tonight to work on this tonight. I'll have time tomorrow night to finish up!

[dbieber]

I'm going to squash and merge what you have so far, and you can put subsequent improvements in a new PR.

[dbieber]

Some next steps:

• raise a FireError if the arg doesn't match any keyword
• give exact matches priority (e.g. if there are args named "a" and "apple" then -a should match "a", rather than being flagged as ambiguous)
• support equal signs with single-character args (-a=10 should be OK)

[dbieber]

Let me know if you're interested in taking on these and if so, which ones

[dbieber]

raise a FireError if the arg doesn't match any keyword

Ah, looking back I see that's actually not how things are currently handled. Unparsed args are returned as part of remaining_kwargs.

[alexshadley]

Yeah, I can start working on supporting equal sings in single-char args.

As far as raising a FireError if the arg doesn't match, how should we proceed? I agree that it would be a nice feature, but as I looked into it further this seems to get tricky. As far as I understand (correct me if I'm wrong here), we first attempt to instantiate the owning class with the arglist, and upon this failing we move on to calling the function. When I attempted to raise an error on no applicable shortcuts, the error was just thrown whenever the class was parsed as the class had no args at all.

[dbieber]

Ah was just about to post here, 1 sec:

[dbieber]

I went ahead and put together a commit to add support for equal signs and giving exact matches priority already, I'll put it on GitHub now and drop a link here.

[dbieber]

As for raising the FireError, I was mistaken about that. The correct behavior (e.g. in order to support --help properly) is to return unused kwargs in the remaining_kwargs entry of _ParseKeywordArgs. I do this in my commit.

Heads up that I don't have it working fully yet. But I'll show you what I have so far.

[dbieber]

0b0f242

Update: fe1fda4 has a fully working version

[alexshadley]

Ah, looks good then! Any other areas of the project that need work done?

[dbieber]

#102 / #97 looks like a fun one if you're up for AST traversals.