Remove check on self.identchars in do_alias() #391
Conversation
self.identchars is no longer used by cmd2.
Codecov Report
@@ Coverage Diff @@
## master #391 +/- ##
==========================================
+ Coverage 89.73% 89.83% +0.09%
==========================================
Files 8 8
Lines 2621 2646 +25
==========================================
+ Hits 2352 2377 +25
Misses 269 269
Continue to review full report at Codecov.
|
|
This change allows the following to work.
Can you add a restriction that prevents creating aliases containing redirection characters? Also, we need to fix alias expansion to allow a redirection character without a space.
This works: |
This is what you get for being too hasty when you push.
|
Given We shouldn't allow Should we allow Should we allow |
# Conflicts: # cmd2/parsing.py # tests/test_parsing.py
|
My thoughts on what we should allow in alias names are as follows:
So basically, I'd say don't allow any of that in alias names to keep things as simple as possible. |
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Consider my comments, but looks good
cmd2/cmd2.py
Outdated
| self.perror("Alias names can only contain the following characters: {}".format(self.identchars), | ||
| traceback_war=False) | ||
| return | ||
| # Validate the alias to ensure it doesn't include wierd characters |
| second_group_items = [re.escape(x) for x in second_group_items] | ||
| # add the whitespace and end of string, not escaped because they | ||
| # are not literals | ||
| second_group_items.extend([r'\s', r'\Z']) |
There was a problem hiding this comment.
Is this the same as invalid_items just with one more item appended? If so recommend making a copy of the former to minimize duplicated code
There was a problem hiding this comment.
Good idea. As I think about this more, I think it would be wise to put both of those regexes into the StatementParser class, and to switch the invalid_alias_pattern to be valid_alias_pattern. That way the logic in using both regexes is the same, i.e. if match: then it's valid. And the logic of generating both patterns would also be encapsulated.
Provide a new is_valid_command() method on StatementParser to determine whether a string of characters could be a valid command. That means it can’t include any redirection, quote chars, whitespace, or terminator characters. This method is used when someone tries to create an alias, to ensure when we try and parse the alias that it will actually parse. This nicely encapsulates and standardizes all the logic for parsing and expansion into the StatementParser class. Also fix a bug in the regex to match valid command names, and add a bunch of new unit tests to ensure the bug stays fixed.
|
Addressed both of @tleonhardt's comments in prior review. Merging to master. |
self.identchars is no longer used by cmd2.