Comment overhaul

[kmvanbrunt]

Removed support for C-style and embedded comments

This reason I simplified our comment support is because of problems running certain commands at the shell.

On windows, this won't work:
!rundll32 bob.dll,#5

Since # is our comment character, the last part of that command is removed. I also can't quote the string because we preserve quotes on commands being passed to other shells and I agree with that approach. In this case rundll32.exe doesn't seem to like the quotes around the string.

Since cmd2 is primarily a shell application, I think embedded comment support is unnecessary. Comments really only are applicable in text scripts, and it's reasonable to have users type comments on their own lines.

Also, C-style comments don't offer any advantage in a text script. I'm OK just letting these go since adding code to prevent them from being embedded will just be more code we have to maintain for a feature I'm not sure if anyone uses.

[codecov]

Codecov Report

Merging #631 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
- Coverage   94.08%   94.07%   -0.01%     
==========================================
  Files          10       10              
  Lines        2957     2954       -3     
==========================================
- Hits         2782     2779       -3     
  Misses        175      175

Impacted Files	Coverage Δ
cmd2/parsing.py	100% <100%> (ø)	⬆️
cmd2/cmd2.py	94.52% <100%> (ø)	⬆️
cmd2/constants.py	100% <100%> (ø)	⬆️

---

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 de70108...1a2465a. Read the comment docs.

[tleonhardt]

Overall it looks good, but please look at my comments.

[tleonhardt]

@kotfu I'd like to make sure we get your input on these two potentially breaking changes. I think they are good chances in terms of simplifying things and making the code more maintainable. But I want to make sure this doesn't negatively impact anything you are working on.

[kotfu]

Nothing I am working on requires either C-style comments or embedded comments. So from that perspective, I'm totally OK with removing this. The only reason I added support for C-style comments when we rewrote the parser is because we had previously supported them. The cmd module in the python standard library doesn't support comments at all, so nothing to worry about there.

Breaking backwards compatibility with previous releases of cmd2 does make me pause a bit. Especially in a patch version change (using the nomenclature of semantic versioning. I know we haven't been following semantic versioning, so I can set that objection aside too.

Our own set -l command implies that we support embedded comments using the # character.

So what if we made the supported commenting styles some sort of setting? I'm even OK with the default being only supporting # comments at the beginning of the line, with a setting to enable embedded # comments, and another setting to enable C-style comments? These settings could be set as immutable [per @tleonhardt's other proposed change to our proliferation of attributes] attributes on the Cmd object, and easily passed into the StatementParser object.

I like the configurable comments because some use cases, like the one @kmvanbrunt outlined, are complicated or impossible if you use python/shell style commenting (either at the beginning of the line or in the middle of the line). In other cases, the C-style comments conflict with the desired input syntax. Configurable comments allows our implementers to choose whether they want no comments, python/shell style comments, C-style comments, or all the comments.

What do you think?

If we decide to do configurable comments, I'm happy to take on the changes to StatementParser.

[kmvanbrunt]

@kotfu Maybe I'm misunderstanding set -l. I thought it just listed the setting with a description. Why do you say it implies the support of embedded comments?

[kotfu]

Because the description is preceded with a ‘#’, making it look like an embedded or inline comment.

…

On Mar 2, 2019, 1:16 PM -0700, kmvanbrunt ***@***.***>, wrote: @kotfu Maybe I'm misunderstanding set -l. I thought it just listed the setting with a description. Why do you say it implies the support of embedded comments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[kmvanbrunt]

@kotfu But that's just the description from the set command output. A user would never type that on the command line since descriptions are not editable. Even if they tried to, argparse would raise an error.

However, if you think it could confuse a user, we can always change the character that precedes the description.

[tleonhardt]

I'd like to steer away from configuration settings that nobody is likely to use. My opinion is that we should just remove support for C-style comments - to me they always felt out of place in a Python framework and I'm not aware of anyone ever having used them. And any of these breaking changes I want to do a release or two before the 1.0.0 release that we are gearing up towards.

I have mixed feelings about embedded Python comments. I see the advantage in terms of simplicity and ease of maintenance of not making it configurable and only allowing full-line comments. But I also see the advantage of making it configurable to either allow in-line comments or not - that would allow the option of backward compatibility and may be useful for some people; though I am also not aware of anyone actually using embedded comments. If we made it configurable, what would the default be?

What are your thoughts?

[kmvanbrunt]

@tleonhardt I understand the desire to not break existing scripts. I just don't think the risk is very high that we will do so by removing embedded comment support.

If we allow the behavior to be configurable, shlex will do the work by setting its comments flag to our flag's value. But as we move toward a 1.0 release, I would like to see less configuration settings that only a minority, if any, of our users care about. I prefer a stable API that is simple to maintain and test.

The approach this PR implements will allow all commands to work without having to toggle a flag. Since the # has to begin the line, it sits in the token reserved for the command name. Therefore it can never conflict with any command set our users think up because commands cannot contain a # character.

[kmvanbrunt]

One more thing. Allowing embedded comments automatically breaks our shell command. As demonstrated in my rundll32 example, there are certain shell commands that we can't run. If a setting has the capability of breaking our own framework, I say it is best to toss it. Especially since calling into the shell is one of our most powerful features.

[tleonhardt]

@kmvanbrunt You make an important point about supporting embedded comments breaking the shell command in some circumstances. Given that, I would advocate at a minimum for making the default behavior to not allow embedded comments. But at that point we are breaking backward compatibility by default anyways and I have to ask myself if having a configuration option is worth it. I'm not sure. I guess I am leaning towards keeping it simple and not making it configurable. But I could be happy with either.

@kotfu Would you be ok with keeping it simple and not making it configurable, or do you feel strongly that there is benefit in making it configurable?

[kotfu]

I'm supportive of dropping C-style comments. The only reason I feel super strongly about having configurable comments is to retain backwards compatibility. If we decide we don't care, then there is no reason to have configurable comments.

I disagree with the notion that these changes make the code more maintainable. We are removing two lines of well-tested, well-encapsulated, and well-documented code.

I disagree that allowing embedded comments "breaks our shell command". Our current approach to comments using the hash symbol (i.e. excluding C-style comments) follows exactly the commenting syntax of sh and all its derivatives, ksh, bash, fish, etc. Comments begin with a '#', and end at the end of a line. If you need a '#' in your command, simply enclose it in either single or double quotes. I don't think it's wise for us to break this convention because there is some command in the world that can't properly handle quoted arguments as input.

I also understand that I'm in the minority in my position, and yield to the decision of the group.

[tleonhardt]

Hmm .. perhaps I am making much ado about nothing. If it is really a matter of a couple/few lines of well-tested, well-encapsulated, well-documented code; then that isn't a maintenance headache.

@kotfu You make a good point about how in general we attempt to support syntax similar to bash and its derivates unless there is a good reason not to.

@kmvanbrunt How about we try adding back in the code to support embedded Python comments based on a configuration called something like allow_embedded_comments. Then we can take a look at it again?

[tleonhardt]

If embedded comments were allowed based on a flag, I would want the default to be to not allow them in order to have maximum support of shell command usage on Windows.

[kmvanbrunt]

@kotfu @tleonhardt Because of how we do shlex parsing, it isn't always as easy as quoting a # character in a string. We set posix to False in shlex which means quotes in a string are treated as literals.

This is from Python's documentation:

Quote characters are not recognized within words (Do"Not"Separate is parsed as the single word Do"Not"Separate);

So if I try: arg_with_"#", shlex treats the # as the beginning of a comment.
Instead you must quote the whole string like: "arg_with_#"

@kotfu is correct that the Unix shells sh and all its derivatives, ksh, bash, fish, etc allow a comment syntax similar to ours. But even that isn't so cut and dry. Here is an example that shows we are similar but not identical to those shells when parsing comments.

In those shells, most would assume this line ends in a comment: touch new#file.
But actually, the # in that case does not start a comment and you end up with a file called new#file.
However If I run !touch new#file in cmd2, shlex parsing treats the # as a comment and you end up passing touch new to the shell.

While that may seem like an obscure case, I don't think it's inconceivable for a user to have a folder named something similar to vacation_photos_#1.

What the example demonstrates is that we aren't totally following sh rules and therefore aren't performing the task expected by the user. That's why I feel strongly about removing support for embedded comments. It's better to just pass the unprocessed string directly to the shell and allow it's own parsing rules to split the string. That provides a seamless user experience where they can type their shell command exactly as they would in the underlying shell, be it Unix or Windows environments.

Our help text for shell is "Execute a command as if at the OS prompt". I believe this PR best accomplishes that objective since the underlying shell does not require users to quote new#file.

I hope this clarifies my earlier statement where I said: "Allowing embedded comments automatically breaks our shell command".

I also just discovered that embedded comments break tab completion of paths. Since a folder called vacation_photos_#1 doesn't have a space, it won't be quoted by tab completion. So tab completing something like !rm -r vacation_photos_#1/ won't produce command text that actually works since #1/ will be stripped by shlex.

I certainly am not fond of breaking anyone's existing scripts. If that happens, I will take the blame. Honestly though, I'm betting we won't see any complaints and if we do, it is a trivial thing for them to move a comment to the line preceding the command. Also, if the user understood the limitations embedded comments add to other functionality of cmd2, I think they would be fine forgoing the feature.

[tleonhardt]

Wow. The more I learn about shells, the more I am surprised to learn that there is still a lot I don’t know! cmd2 does a very admirable job of providing an excellent shell-like application environment on both POSIX and Windows OSes, which isn’t easy.

I don’t like breaking backwards compatibility, but it seems like there are good reasons in this case.

Let’s do our best to get to a very stable base for the 1.0 release where we have extremely few breaking changes after that.

[tleonhardt]

Well, this one has had some healthy debate ;-) I'm not 100% sure we are making the right choice, but I'm going to merge and and move forward. If we find any gotchas, we can add back in embedded comments.