decorator changes for replacing optparse with argparse #249

kotfu · 2018-01-13T06:16:56Z

This PR adds a new @use_argument_list decorator which converts do_* methods to get passed a list of strings instead of a string. This is in lieu of using the @with_argument_parser decorator for full-blown argparse parsing.

This closes #47
This closes #250
This closes #251
This closes #253
This closes #252

new attribute on Cmd2.cmd which defaults to false, but if set true, causes all do_* commands to receive a list of arguments, instead of a string of what the user typed.

codecov · 2018-01-13T06:23:00Z

Codecov Report

Merging #249 into master will decrease coverage by 0.58%.
The diff coverage is 96.72%.

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   96.45%   95.87%   -0.59%     
==========================================
  Files           1        1              
  Lines        1241     1235       -6     
==========================================
- Hits         1197     1184      -13     
- Misses         44       51       +7

Impacted Files	Coverage Δ
cmd2.py	`95.87% <96.72%> (-0.59%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6895dea...1da9045. Read the comment docs.

tleonhardt

As discussed in the comments, we can't use the use_argument_list attribute and have it modify onecmd() because that breaks compatibility with methods implemented by the underlying cmd module, most importantly help.

tleonhardt · 2018-01-13T14:36:59Z

cmd2.py

    excludeFromHistory = '''run ru r history histor histo hist his hi h edit edi ed e eof eo eos'''.split()
    exclude_from_help = ['do_eof', 'do_eos']  # Commands to exclude from the help menu
    reserved_words = []
+    use_argument_list = False


This class member shouldn't be added. It creates massive backwards compatibility problems with cmd.

tleonhardt · 2018-01-13T14:41:06Z

cmd2.py

            return self.default(statement)
-        stop = func(statement)
+
+        if self.use_argument_list:


We can't do this in onecmd. This breaks any underlying calls which are actually implemented by Python's built-in cmd module which cmd2 extends - most importantly help. With this in place doing a "help history" will throw a "TypeError" exception because the underlying cmd help commands expects a string.

Additionally, if someone tries to upgrade their cmd app to use cmd2, at first none of their commands would work.

tleonhardt · 2018-01-13T14:42:43Z

cmd2.py

    return arg_decorator


+def with_argument_list(func):


I like this decorator-based approach for getting cmd2 to pass the do_* methods an argument list instead of an argument string. It is an easy way to alter the default behavior on a per-function basis.

tleonhardt · 2018-01-13T14:45:36Z

docs/argument_processing.rst

+The default behavior of ``cmd2`` is to pass the user input directly to your
+``do_*`` methods as a string. If you don't want to use the full argument parser support outlined above, you can still have ``cmd2`` apply shell parsing rules to the user input and pass you a list of arguments instead of a string. There are two methods to effect this change.
+
+The ``use_argument_list`` attribute of a ``cmd2`` class or subclass defaults to ``False``. Set it to ``True`` and all of your ``do_*`` methods will receive a list of arguments parsed by ``shlex.split()`` instead of receiving a string::


Update this documentation once the use_argument_list attribute has been removed.

tleonhardt · 2018-01-13T16:12:37Z

On second thought, including the use_argument_list attribute and modifications to onecmd() can be ok. But if we keep them, we need to put in a modification to do_help() so that the underlying call to cmd.Cmd.do_help() always gets a str even when use_argument_list is True. So basically we would need to put in a "''.join()'' to do the conversion.

But it is possible that there may be other underlying calls to methods in cmd that I am not thinking of, and if that is the case then the user_argument_list attribute would still be problematic.

kotfu · 2018-01-13T17:18:41Z

So to keep the use_argument_list attribute, we need to go find all occurrences of do_* methods in both cmd.cmd and Cmd2.cmd and make sure they still work properly. After looking at do_help(), that one will be easy. But to continue on this path we need to find them all. I think I'll investigate using introspection in onecmd() to check if the method we are about to call is part of cmd.cmd or Cmd2.cmd, and if so, pass the string instead of the argument list. That way if in the future we add a new do_* method to Cmd2.cmd we won't create an unexpected bug.

If this still turns out to be too problematic, perhaps we can try creating a new subclass of str which has a arglist() method. Pass an instance of this new class to the do_ methods where we are currently passing a plain string. Existing code works fine, if you want a list of arguments, call the arglist() method.

tleonhardt · 2018-01-13T17:38:23Z

Yes, to keep the use_argument_list attribute, we need to find all occurrences of do_* methods in cmd2.Cmd and make sure they work properly both when that argument is True and False. Additionally, we would need something like you mention with introspection within onecmd() so that if the underlying method call is to something within cmd.Cmd, that it always gets passed a string instead of a list. And we would need a tweak within the cmd2 override of do_help.

Many of the do_* methods in cmd2 currently use the @options decorator and have a snippet of code within them such as the following to behave one way when a str is passed and another way when a list is passed:

# If arguments are being passed as a list instead of as a string
if USE_ARG_LIST:
    if arg:
        arg = arg[0]
    else:
        arg = ''

@kotfu
What do you think it would be better in the long run?

Just get rid of use_argument_list and avoid the associated compatibility issues?
Make all of the changes discussed to deal with the compatibility issues?

kotfu · 2018-01-13T18:50:27Z

@tleonhardt After looking at some of the do_* methods in cmd2.py, I thought of another potential solution. Most of the do_* methods in cmd2.py (i.e. do_set(), do_pyscript()) have some way of parsing arguments. Some use *@options, others use pyparsing, others just do custom parsing. If we migrate all of that argument parsing to our new argparse based decorator, that avoids any potential issues with handling plain string vs arguments as a list. Issue #250 contains all the details.

Since I think we have to do issue #250 anyway, at this point I'm inclined to try and implement the changes we have discussed to make use_argument_list work, and see how ugly it is. If it's horrible, then I think we just abandon use_argument_list. If it's not horrible, and if I can do it in a way that's not too brittle, then I think we should use it.

kotfu · 2018-01-13T18:54:18Z

@tleonhardt, I don't understand your prior comment: 'Additionally, if someone tries to upgrade their cmd app to use cmd2, at first none of their commands would work.' Can you better explain your concern?

tleonhardt · 2018-01-13T20:53:44Z

@kotfu
Thanks for creating #250. We do need to replace the use of the @options decorator since we are calling that deprecated.

Yeah, definitely go forward with the use of use_argument_list and implement the changes as discussed. Hopefully there won't be any gotchas we didn't think about.

As long as the default value of use_argument_list is False, I think my previous concern is unfounded. We just need the default behavior for do_* methods without any decorator to be consistent with cmd where they get passed a single argument which is a str. My concern was that if they did set use_argument_list to True, it would break any existing do_* methods in their cmd app when they port it to cmd2. But as long as the default value is False, then I'd say my concern was much ado about nothing.

This commit converts the following methods: - do_eos() - do_eof() - do_quit() - do_shortcuts() - do_cmdenvironment() - do__relative_load() - do_load() - do_help()

kotfu · 2018-01-15T02:22:26Z

While working on Issue #250, I discovered that we will have to discard the use_argument_list attribute. It breaks do_shell() and do_py(). I'm gonna think a bit more about this and see if I can come up with an alternative solution, but as of right now, I think we will have to stick with the decorators only.

tleonhardt · 2018-01-15T02:48:32Z

Now that we have a 2nd decorator which simply converts a do_* command so that it gets passed an argument list instead, I'm pretty happy with the decorator approach. That feels to me like a relatively painless, explicit, and self-documenting API.

If you can think of a smart and safe alternative to the use_argument_list attribute which gives similar functionality, I'd be curious to see the approach. But I wouldn't recommend spending too much time thinking about it.

This one depends on the help text for history

tleonhardt · 2018-01-15T05:33:43Z

@kotfu I hope me pushing a few minor fixes doesn't get in the way of what you are doing

argparse output for invalid option has different text and goes to stderr

… history"

It's functionality has been moved inside the set command. The set command now uses an argparse parser.

…t input

…sts as a -a/--all option to the set command

@options

…ted @options optparse-based decorator

tleonhardt · 2018-01-15T21:12:25Z

Ok, I think I'm done working on #251 and #253.

Let me know when you push changes for #252 and I'll take a look at them. That one is a little more involved. But once we wrap that up, I think we should finally merge this PR.

…was merged into the "history" command awhile back

tleonhardt · 2018-01-16T18:35:14Z

I like how the new and improved history command is behaving. FYI, I think it is "cowardly", not "cowerdly".

So now to complete this PR we just need to remove the deprecated run and save commands and remove the command editing capability from the edit command.

Random train of thought for another time/PR. Now that history command has the ability both to execute previous commands and to output previous commands to a file, it might be cool to add the capability for it to automatically generate a transcript. I don't know how hard that would be, but if it was easy, that would be a really useful feature to add.

kotfu · 2018-01-17T04:49:41Z

@tleonhardt I think we've got everything done. #252 is completed, and I fixed the spelling. I just updated the changelog, and I think this PR is ready to merge.

tleonhardt · 2018-01-17T13:52:49Z

CHANGELOG.md

-            * The old decorator is still present for now, but should be considered *deprecated* and will eventually be removed
-        * See the **Argument Processing** section of the documentation for more information
-            * Alternatively, see the **argparse_example.py** example
+    * Three new decorators for **do_*** commands to make argument parsing easier


Thanks for updating the Changelog

Removed usage of and reference to attributes and commands which have now been removed.

tleonhardt · 2018-01-17T14:13:14Z

README.md

 # regexes on prompts just make the trailing space obvious
 (Cmd) set
 abbrev: True
-autorun_on_edit: False


My recent commit was just a minor cleanup of documentation and such

kotfu added 3 commits January 12, 2018 21:07

@with_argument_parser now passes an arglist instead of a string

19c0586

new @with_argument_list decorator

c26b008

add use_argument_list setting

405f4e4

new attribute on Cmd2.cmd which defaults to false, but if set true, causes all do_* commands to receive a list of arguments, instead of a string of what the user typed.

kotfu requested a review from tleonhardt January 13, 2018 06:16

tleonhardt requested changes Jan 13, 2018

View reviewed changes

kotfu added 4 commits January 14, 2018 15:38

more robust argparse and arglist decorators

cab8a16

Ensure help function works if use_argument_list = True

c69216b

Convert built-in commands to argparsing for #250

144ebaa

This commit converts the following methods: - do_eos() - do_eof() - do_quit() - do_shortcuts() - do_cmdenvironment() - do__relative_load() - do_load() - do_help()

refactor onecmd() to use parse_quoted_string()

22b4f4a

tleonhardt added this to the 0.8.0 milestone Jan 15, 2018

kotfu and others added 7 commits January 14, 2018 20:54

remove use_argument_list attribute

eac6929

convert do_history() to argparse

0e77f4b

convert do_show() to argparse

fda7300

Fixed @with_argument_decorator so that built-in help text is preserved

ddac998

Fixed do_history unit tests and implementation

07e37b2

convert do_pyscript() to use @with_argument_list

6454f4e

Merge branch 'arglist' of github.com:python-cmd2/cmd2 into arglist

eb12773

kotfu changed the title ~~decorator and attribute to resolve issue #47~~ decorator and attribute to resolve issues 47 and 250 Jan 15, 2018

Fixed test_echo unit test

6b5d362

This one depends on the help text for history

Fixed base_invalid_option unit test

2fcc883

argparse output for invalid option has different text and goes to stderr

tleonhardt added 7 commits January 15, 2018 13:56

__relative_load command is now hidden from help menu by default

8d9c281

Fixed expected value of "help history" for unit tests

f2d887f

Made test_echo unit test robust to changes in expected value of "help…

a8e79fd

… history"

show command has been removed

1d1c55b

It's functionality has been moved inside the set command. The set command now uses an argparse parser.

Fixed the test_transcript unit test when run with the from_cmdloop.tx…

b89cbb4

…t input

cmdenvironment command has been removed and its functionality now exi…

f7cc161

…sts as a -a/--all option to the set command

Revised some of the Sphinx documentation which referenced the depreca…

d588bd1

…ted @options optparse-based decorator

tleonhardt changed the title ~~decorator and attribute to resolve issues 47 and 250~~ decorator changes related to replacing optparse with argparse Jan 15, 2018

tleonhardt changed the title ~~decorator changes related to replacing optparse with argparse~~ decorator changes for replacing optparse with argparse Jan 15, 2018

Fix test_base_show_readonly unit test on Windows

e14f628

tleonhardt added enhancement major documentation labels Jan 15, 2018

tleonhardt and others added 3 commits January 15, 2018 17:20

Fixed comment in README.md that referred to the "list" command which …

e3bc19f

…was merged into the "history" command awhile back

Implement -o, -r, and -e options for issue #252

cb7269f

tweak unit tests for history #252

60890fa

kotfu added 3 commits January 16, 2018 20:59

Remove do_save() and do_run() for #252

d8ade12

do_edit() no longer edits history, just files #252

fd5750f

Update changelog with #252 changes

01596cb

Fix bug which prevented multiple history commands from being run

51e7176

kotfu mentioned this pull request Jan 17, 2018

All commands should go through same shlex parsing #47

Closed

tleonhardt previously approved these changes Jan 17, 2018

View reviewed changes

Cleanup of documentation and examples

1da9045

Removed usage of and reference to attributes and commands which have now been removed.

tleonhardt dismissed their stale review via 1da9045 January 17, 2018 14:07

tleonhardt approved these changes Jan 17, 2018

View reviewed changes

tleonhardt merged commit 91bc999 into master Jan 17, 2018

tleonhardt deleted the arglist branch January 17, 2018 14:28

decorator changes for replacing optparse with argparse #249

decorator changes for replacing optparse with argparse #249

Uh oh!

Conversation

kotfu commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tleonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

tleonhardt Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

tleonhardt Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

tleonhardt Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

tleonhardt Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

tleonhardt commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotfu commented Jan 13, 2018

Uh oh!

tleonhardt commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotfu commented Jan 13, 2018

Uh oh!

kotfu commented Jan 13, 2018

Uh oh!

tleonhardt commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotfu commented Jan 15, 2018

Uh oh!

tleonhardt commented Jan 15, 2018

Uh oh!

tleonhardt commented Jan 15, 2018

Uh oh!

tleonhardt commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tleonhardt commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotfu commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tleonhardt Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

tleonhardt Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kotfu commented Jan 13, 2018 •

edited

Loading

codecov bot commented Jan 13, 2018 •

edited

Loading

tleonhardt commented Jan 13, 2018 •

edited

Loading

tleonhardt commented Jan 13, 2018 •

edited

Loading

tleonhardt commented Jan 13, 2018 •

edited

Loading

tleonhardt commented Jan 15, 2018 •

edited

Loading

tleonhardt commented Jan 16, 2018 •

edited

Loading

kotfu commented Jan 17, 2018 •

edited

Loading