Skip to content

Conversation

@nrhinehart
Copy link

No description provided.

@neithere
Copy link
Owner

neithere commented Aug 7, 2014

Hi @nrhine1, thanks for the contribution. Two things:

  1. Quality is extremely important. This is why I only accept pull requests and patches if comprehensive tests are included. Otherwise I'd have to write the tests first; this would significantly increase the required effort to understand and process the proposal and therefore decrease the probability that I would allocate time any soon for the issue.
  2. I'm not sure that I understand the point of this PR. If you are doing mutually exclusive arguments, that's great but I didn't see it in the code (would be easier with tests). If you are just toggling booleans, doesn't Argh do it already? Again, tests would help a lot.

@nrhinehart
Copy link
Author

Hi @neithere,

  1. I've added a set of tests for the two added decorators, which should
    help clarify use cases.
  2. The primary use case is to handle the fact that when a boolean is meant
    to trigger some type of action, like "do_foo" and defaults to True, then
    the only way to set it to False is to specify --do-foo, which is extremely
    counterintuitive. With the decorator, mutually exclusive arguments for
    negation can be used, and with the usage of, e.g.:

@argh.set_toggleable('--do-foo', inv_prefix = 'dont')

the user can use: --dont-do-foo
in order to flip the default and set do_foo to False ,
and additionally, using --do-foo will do what a user might expect, and set
do_foo to True.

I will close this pull request and add another that contains all of my
changes rebased into a single commit

On Thu, Aug 7, 2014 at 7:06 AM, Andy Mikhailenko notifications@github.com
wrote:

Hi @nrhine1 https://github.com/nrhine1, thanks for the contribution.
Two things:

  1. Quality is extremely important. This is why I only accept pull requests
    and patches if comprehensive tests are included. Otherwise I'd have to
    write the tests first; this would significantly increase the required
    effort to understand and process the proposal and therefore decrease the
    probability that I would allocate time any soon for the issue.
  2. I'm not sure that I understand the point of this PR. If you are doing
    mutually exclusive arguments, that's great but I didn't see it in the code
    (would be easier with tests). If you are just toggling booleans, doesn't
    Argh do it already? Again, tests would help a lot.


Reply to this email directly or view it on GitHub
#72 (comment).

Nick Rhinehart
Graduate Student
CMU Robotics Institute

@nrhinehart nrhinehart closed this Aug 7, 2014
@neithere
Copy link
Owner

Now I get the idea. Usually I just add the argument --no-foo. However, resulting statements like if not no_foo: ... in function body indeed have a negative impact on readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants