Skip to content
This repository has been archived by the owner on Apr 22, 2021. It is now read-only.

make paths positional arguments #29

Merged
merged 1 commit into from
Aug 29, 2016
Merged

make paths positional arguments #29

merged 1 commit into from
Aug 29, 2016

Conversation

paiweilai
Copy link
Contributor

@paiweilai paiweilai commented Aug 27, 2016

Click is a Python package for creating beautiful command line interfaces.
This pull request resolves #27.

@paiweilai
Copy link
Contributor Author

screen shot 2016-08-26 at 20 54 56

@paiweilai
Copy link
Contributor Author

Might need to write more tests to pass the coverage ... :(

@asottile
Copy link
Contributor

asottile commented Aug 27, 2016

Why use click when you can accomplish the same thing with stdlib argparse?

@ajm188
Copy link
Contributor

ajm188 commented Aug 27, 2016

Agreed with @asottile. This feature can be implemented using argparse posargs

@paiweilai paiweilai changed the title use click for cli make paths positional arguments Aug 27, 2016
@paiweilai
Copy link
Contributor Author

paiweilai commented Aug 27, 2016

I was playing around with Click and thought it might be useful.
Anyway, I changed the code back to use argparse.
I'll work on getting test coverage 100% in a separate PR.

screen shot 2016-08-27 at 08 45 38

@asottile
Copy link
Contributor

btw, I think we can turn off the annoying coveralls comments unless you guys find them useful.

@ajm188
Copy link
Contributor

ajm188 commented Aug 27, 2016

That still leaves the "check" on pull requests enabled, right? If so, I vote yes.

@paiweilai
Copy link
Contributor Author

I vote yes, too. So noisyyyyy.

@asottile
Copy link
Contributor

I've disabled the comments

@paiweilai paiweilai self-assigned this Aug 27, 2016
parser.add_argument(
'--pattern', '-p', metavar='path', action='append', required=True,
'--pattern', '-p', metavar='PATH', action='append', required=True,
help='paths to pattern definition files')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: shouldn't this read "path to pattern definition file" rather than "paths ... to files" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change it because it can take multiple patterns as far as I know.

@paiweilai
Copy link
Contributor Author

paiweilai commented Aug 28, 2016

@asottile @ajm188 @evhub I'll do the following according to the discussion.

  1. use --jobs instead of --multiprocess for now; if we want to remove this feature, work on this in a separate PR.
  2. use PATH for inputs for now; if we want to remove directory traversal, work on this in a separate PR and change PATH to FILENAME after the feature is removed.
    I'll merge the branch if no objections.

@@ -49,21 +49,21 @@ def _read_output_file():


def test_single_file():
args = ["undebt", "-i", method_to_function_input_path, "-p", method_to_function_path, "--verbose"]
args = ["undebt", "-p", method_to_function_path, "--verbose", method_to_function_input_path]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this and the other tests to move the --verbose to the end? I'd like to make sure that if the arguments are of the form undebt -p pattern input that input isn't treated as another pattern, but this test doesn't check that, since the --verbose is in the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@asottile
Copy link
Contributor

Accepting -i and ignoring it (store_true action probably) will establish backwards compatibility (the non flag arguments will still end up in the nargs)

@paiweilai
Copy link
Contributor Author

@asottile Yes, your solution will work. However, I don't see the point for keeping backward compatibility... If we really care, then I also should not rename --multiprocess to --jobs.

@paiweilai paiweilai merged commit f764ebf into Yelp:master Aug 29, 2016
@paiweilai paiweilai mentioned this pull request Aug 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify CLI to support input paths as positional arguments
4 participants