Get rid of argparse usage entirely.

Review Request #3074 — Created Nov. 4, 2015 and submitted

benjyw
pants
a72a192...
pants-reviews
molsen, nhoward_tw

It had become an implementation detail, and a minor one at that.
We had already duplicated a lot of its logic (around types and actions,
in particular) in order to correctly compute defaults. So we might as
well go the extra mile and get rid of it. Especially since its init
sequence is quite expensive, and we create one argparser per scope,
which is not how it was designed to be used.

This meant we had to implement a couple of extra bits of functionality
that argparse did previously handle for us:
- choices
- nargs=?. Instead of implementing nargs fully, we now allow
an implicit_value= kwarg, which provides the value if you
specify a flag with no value (e.g., --foo instead of --foo=bar).
This is the only thing we were using nargs for, and even that
only in one place.

This allows us to simplify OptionValueContainer, as we can now guarantee
that all values set on it are of type RankedValue.

Note that this breaks one little piece of functionality that we
never wanted to begin with: argparse recognizes any unambiguous prefix
of an option. E.g., if we register --foobar, and there's no other option
that starts with --foo, then you only need to use --foo on the cmd line.
This is a very bad feature, because it means command lines will break
as soon as you add a new option with the same prefix. Sadly, there was
no way to turn it off. So it's possible that someone somewhere is
relying on this bad behavior, which is now gone. In which case they
will get an "unknown option" error, and will simply have to spell the
option correctly. They were on thin ice to begin with, so this is not
worth worrying about.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/89366469

  • 0
  • 0
  • 9
  • 2
  • 11
Description From Last Updated
MO
  1. 
      
  2. src/python/pants/option/parser.py (Diff revision 1)
     
     

    Since you aren't specifying a default why aren't you just using kwargs['implicit_value']. Should we have a default here?

    1. The "default default" for dict.get() is None. kwargs['implicit_value'] will throw a KeyError if the user didn't specify an implicit value (which they almost never will).

    2. Makes sense

  3. src/python/pants/option/parser.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This has a pretty high cyclomatic complexity for this method. Could you break this up into smaller testable chunks so this is easier to reason with and test?

    1. I have no idea what "cyclomatic complexity" means :) but I cleaned this function up a bunch, so thanks for the suggestion. I like it a lot better now.

      Most of the new pieces are not directly testable, because they're defined inside the function, but that's OK - this is very low-level stuff, and it's all tested via higher concepts.

    2. I only see one revision, did you push the changes to the RB?

    3. Just now...

  4. src/python/pants/option/parser.py (Diff revision 1)
     
     

    Since this is only using values from the parent loop it seems like there is a chance of it being called more than once in this loop. This seems like we have the potential of writting out dup lines in warnings.warn.

    1. It's now called in a function called "consume_flag()" which makes it a bit more obvious that this can only be called once per option (and that logic is now easier to follow).

  5. src/python/pants/option/parser.py (Diff revision 1)
     
     

    I assume users can still specify advanced, fingerprint, etc. This seems like useful information to have documented in the docstring.

    1. There definitely needs to be documentation of all the registration kwargs, but this isn't the right place for that documentation though, as Parser isn't a class anything outside pants.option is supposed to interact with (except in a couple of cases, e.g., help generation). Meanwhile I wanted to get rid of any suggestion that we support argparse args... We support our args, some of which happen to be the same as argparse... I will write up some documentation in a separate change, but in the Option class, or in an .md file directly.

  6. src/python/pants/option/parser.py (Diff revision 1)
     
     

    Can we add the specific check in to fail on nargs in case someone is using this in plugins somewhere or something. As far as I can tell we will just ignore it right now, which means it will be hard to find code doing this.

    1. There is already a check for any unrecognized kwarg, and an attempt to use nargs will fail on that. See _allowed_registration_kwargs.

  7. src/python/pants/option/parser.py (Diff revision 1)
     
     
     
     
     
     
     
     

    Probably a personal pref but, I find it harder to red code that has a def in the middle of a block of code. Also since its really limited to the scope of the calling method you may as well call it _check for clarity. Could you move this to the beginning of the method def?

    1. I actually prefer to define this close to where it's used, so I think it's better where it is Calling it _check actually detracts from clarity: The leading underscore is a python convention for "weakly private member", for example import * won't import module members with a leading underscore. This inner function is not a member of anything.

      From PEP8: Use one leading underscore only for non-public methods and instance variables.

    2. I would argue this is a non public method since it is only defined within the scope of the calling method. There is no way to access this method outside of its parent.

    3. It's definitely non-public, but it's not a method. It's a freestanding function. The underscore convention is to warn the user that they should not access this member outside the class, even though they could. This function couldn't be accessed at all, so it's not a problem.

  8. tests/python/pants_test/option/test_options.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can this be split into multiple tests or tested in a way that allows you to see all failure cases if something goes wrong?

    1. We've had this discussion before, these are too fine-grained to be worth the extra boilerplate. And the value of seeing all failure cases here is debatable at best. Some may like it, others (myself included) may find it more distracting than helpful. In any case, that's how most of our tests are structured.

  9. tests/python/pants_test/option/test_options.py (Diff revision 1)
     
     
     
     
     
     

    Same comment as previous test

  10. suggest changing the name to test_RegistrationError to match test

    1. I don't get it. Why would it be named test_RegistrationError? Or was that a copy-paste mishap?

      It's currently named precisely after the functionality it tests.

    2. Sorry my brain flipped left/right old/new

    3. Shakes fist at RB.

  11. 
      
BE
MO
  1. Great thanks for getting these changes out. LGTM.

  2. 
      
NH
  1. Looks good. This'll clear up a lot of confusing behavior from argparse! I've got some minor suggestions and a couple comments.

  2. src/python/pants/option/parser.py (Diff revision 2)
     
     

    Add scope str in error msg here too?

  3. src/python/pants/option/parser.py (Diff revision 2)
     
     

    Maybe throw the action set in a member like allowed kwargs?

    1. Good idea, done.

  4. src/python/pants/option/parser.py (Diff revision 2)
     
     

    nit: ws ), fromfile

    1. Good eye, fixed.

  5. src/python/pants/option/parser.py (Diff revision 2)
     
     

    What happens if the append option's ranked values are all either empty lists or None?

    filter(None, [[]]) == []

    1. Ah my bad, that's cruft. That filter doesn't actually do anything because the items in that list are RankedValues, so they are never falsy. Also, that list is guaranteed to have at least one element. I added a comment to clarify, and a test case, for good luck.

  6. How would you feel about renaming this to check_failure? I think it'd make the block of assertions below clearer.

    1. Renamed it assertError, to make it clearer that it asserts.

  7. I'm not sure what this comment is referring to. Are there strings in the assertions somewhere I don't see?

    1. Ha, that comment was left over from a previous version that just checked the messages on RegistrationError instances. Then I decided that I didn't agree with my own comment, and went ahead and created said subclasses (since I realized it was easy to do so with a factory function and almost no boilerplate). Deleted the now-irrelevant comment.

  8. 
      
BE
BE
NH
  1. Ship It!
  2. 
      
BE
  1. Thanks! Submitted as d6b1ade52fe313bd853667ea365c110ed06fc74a.

  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

d6b1ade52fe313bd853667ea365c110ed06fc74a

Loading...