Improve parser #25
Improve parser #25
Conversation
* Add behave short options to parser * Fix specifying paths bug * Customize parser's usage and description text to match behave's * Add short option (-k) to --keepdb argument
| * Added :code:`--keepdb` short form argument, `-k` | ||
|
|
||
| **Bugfixes** | ||
| * Fixed specifying paths didn't work |
There was a problem hiding this comment.
There is a blank link missing after "Bugfixes", otherwise the bulleted list is not interpreted correctly.
Also, please use - signs instead of * for bullets, to keep the markup uniform across the document. 👍
| '--version', | ||
| '-c', | ||
| '-k', | ||
| '-v' |
There was a problem hiding this comment.
Sorry to be picky, can we put a , (comma) at the end of each list item if it's on a single line? This way we never have changes in existing lines when elements are added.
| # Do not add conflicting options | ||
| if long_option in conflicts: | ||
| # No use in adding an option if it doesn't have an option string | ||
| if len(option_strings) == 0: |
There was a problem hiding this comment.
How about ... instead?
if not option_strings:I have been told this is more pythonic.
Also, consider rewording the comment above to make it sound more positive. Or, even better, omit it if the code is self-explanatory.
There was a problem hiding this comment.
Actually, what do you think about renaming the long option strings with --behave instead of outright removing them?
E.g. --behave-version
This will only apply to commands where both the short and long option strings are conflicting.
There was a problem hiding this comment.
If that means prefixing only the conflicting options then yes, I agree. 👍
|
|
||
| [pytest] | ||
| [tool:pytest] |
There was a problem hiding this comment.
Why? Shouldn't this be [pytest] according to the py.test documentation?
There was a problem hiding this comment.
The latest pytest gives me a deprecation warning. Here's their PR for reference.
There was a problem hiding this comment.
As soon as a new version of behave is released we could actually consider moving the whole content of setup.cfg to tox.ini.
All tools we use for testing also read their sections out of tox.ini, including behave in its development version.
|
| if hasattr(keywords.get('type'), '__call__'): | ||
| del keywords['type'] | ||
|
|
||
| # config_help isn't a valid keyword for make_option |
There was a problem hiding this comment.
The comment is misleading: make_option is not used anymore. This is a leftover from the implementation with optparse. (Sorry, probably my fault.) -- Is this check still necessary, btw?
There was a problem hiding this comment.
I don't think it is. Seems to work without it.
| behave_args.append(option.replace('--behave', '')) | ||
| else: | ||
| # Strip behave prefix from long options | ||
| behave_args.append(option.replace('behave-', '')) |
There was a problem hiding this comment.
This is hard to read. We need comments to explain the code (which seems to support my impression).
How about:
if option.startswith('--behave-'):
option = option.replace('--behave-', '')
prefix = '-' if len(option) == 1 else '--'
behave_args.append(prefix + option)There may be a better way.
|
|
||
| **Features and Improvements** | ||
|
|
||
| - Behave's short form arguments are now accepted (e.g. :code:`-i` for :code:`--include`) |
There was a problem hiding this comment.
Please add:
- Prefix conflicting command line options with :code:`--behave`
| if 'config_help' in keywords: | ||
| del keywords['config_help'] | ||
| # Build option strings | ||
| # Conflicting option strings get prefixed with `--behave` |
There was a problem hiding this comment.
How about moving this specific comment two lines down? -- We've got a superabundance of comments here.
| if option.startswith('--'): | ||
| option = '--behave-' + option[2:] | ||
| else: | ||
| option = '--behave-' + option[1:] |
There was a problem hiding this comment.
The second level of if, beside (visually) increasing complexity, is visually non-DRY.
How about:
if option in conflicts:
option = '--behave-' + option[2 if option.startswith('--') else 1:]or -- probably more readable and self-explanatory:
if option in conflicts:
prefix = '--' if option.startswith('--') else '-'
option = option.replace(prefix, '--behave-', maxreplace=1)
|
| for option in unknown: | ||
| # Remove behave prefix | ||
| if option.startswith('--behave-'): | ||
| option = option.replace('--behave-', '') |
There was a problem hiding this comment.
We probably should add a 1 (maxreplace) parameter here too, to be on the safe side. Sorry!
| description = """\ | ||
| Run a number of feature tests with behave.""" | ||
| parser.usage = usage | ||
| parser.description = description |
There was a problem hiding this comment.
Hmmm, is there a reason why you declare usage and description, only to assign them immediately afterwards? Would a direct assignment not be good enough? Just wondering why, really.
parser.usage = "%(prog)s [options] [ [DIR|FILE|FILE:LINE] ]+"
parser.description = """\
Run a number of feature tests with behave."""(Sorry for the never-ending story! I promise to stop looking at the code now!) 😏
|
Sorry for the delay. I got busy, then got sick, and now it's Christmas. Haha. I'll get back to this as soon as I have some free time. 👍 |
|
Hopefully this is the commit that closes this PR! 😂 |
|
A new release would be nice now. |
Improve parser
e.g. -d for --dry-run, -e for --exclude
Before this PR, specifying a path will not work since our parser didn't have an
option for positional argument. This PR adds a
pathsargument.The default usage and description was very messy. This PR updates them to be
like behave's.