Skip to content

Conversation

@kmvanbrunt
Copy link
Member

This is pretty much a refactor that provides a common way of calling shlex.split() to ensure the same arguments are always used for consistent parsing.

I also replaced parse_quoted_string() with _get_command_arg_list(). I believe the name and documentation of this function better communicates its purpose. This function also doesn't bother calling shlex.split() on an already parsed Statement object.

@kotfu I removed a unit test called test_arglist_decorator_twice. Is that an OK change? I wasn't sure the use case of having two with_argument_list decorators on one method. Let me know if I need to put it back and make the new code work with it. It would be a trivial fix for me.

Replaced parse_quoted_string with _get_command_arg_list.
@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #638 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #638      +/-   ##
==========================================
+ Coverage   94.22%   94.23%   +0.01%     
==========================================
  Files          11       11              
  Lines        3014     3020       +6     
==========================================
+ Hits         2840     2846       +6     
  Misses        174      174
Impacted Files Coverage Δ
cmd2/parsing.py 100% <100%> (ø) ⬆️
cmd2/cmd2.py 94.22% <100%> (-0.02%) ⬇️

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 04eac4b...38804d7. Read the comment docs.

Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

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

See my comments

from .argparse_completer import AutoCompleter, ACArgumentParser, ACTION_ARG_CHOICES
from .clipboard import can_clip, get_paste_buffer, write_to_paste_buffer
from .parsing import StatementParser, Statement, Macro, MacroArg
from .history import History, HistoryItem
Copy link
Member

Choose a reason for hiding this comment

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

Changes look good

@kmvanbrunt kmvanbrunt merged commit d6c6cf3 into master Mar 6, 2019
@kmvanbrunt kmvanbrunt deleted the common_split branch March 6, 2019 14:04
@kotfu
Copy link
Member

kotfu commented Mar 7, 2019

Now that it's merged, here's my comment on the question @kmvanbrunt asked above, i.e. what's the use case for duplicate argils decorators on a single method. It was just an application of Postel's law, which is: be conservative in what you do, be liberal in what you accept from others. I have no use case for why you would need two decorators on the method, but since it was trivial to ensure that our decorator didn't break or give unexpected behavior if someone did apply it twice, I tested for it and make sure it worked.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants