Skip to content

Conversation

@kotfu
Copy link
Member

@kotfu kotfu commented Apr 26, 2018

When I created this branch, I thought we should use ply to parse user input. After struggling to make it happen, I switched to shlex, and it worked out much better. It's a pain to rename branches, so I didn't.

This PR switches the parsing engine from pyparsing to shlex as described in #37, which reduced the time required to run the test suite by more than 3 orders of magnitude, resolving issue #287. I also removed POSIX_SHLEX and STRIP_QUOTES_FOR_NON_POSIX to close #327.

There are a few items remaining before this PR should be merged:

  • Consider refactoring complete() to use StatementParser.parse() instead of parseline(). We may need to create a new method in StatementParser to get the desired behavior, because tab completion needs to work even if there are current syntax errors in the line of user input.
  • Refactor parseline() to use StatementParser.parse()
  • Remove all use of pytest-xdist and pytest-forked from tox.ini for running the unit tests
  • Update CONTRIBUTING guide to remove all reference to pytest-xdist and pytest-forked
  • Consider doing a full code coverage analysis for all test runs since tests are now much faster
  • Consider adding more versions of Python for Windows tests on AppVeyor
  • Update CHANGELOG.md with changes accumulated in SHLEX_TODO.txt
  • delete SHLEX_TODO.txt

Due to the extensive nature of these changes, I welcome any additional manual testing that anyone is able to do.

@kotfu
Copy link
Member Author

kotfu commented Apr 30, 2018

Aliases worked fine, but shortcuts nested in aliases didn't get expanded properly. Fixed, with some new unit tests to ensure it works in the future.

tleonhardt
tleonhardt previously approved these changes Apr 30, 2018
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.

aliases are now working fine for me. Thanks!

re.DOTALL | re.MULTILINE
)

# aliases have to be a word, so make a regular expression
Copy link
Member

Choose a reason for hiding this comment

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

aliases now work great

kotfu and others added 4 commits April 30, 2018 16:43
Changes include:
- Removed support for versions of setuptools prior to 18.0 (dating to early 2015)
    - This removed some extra logic related to conditional dependencies and simplified the imports
- Added a python_requires statement to require Python 3.4 or newer
    - I believe this requires setuptools >= 34.4
tleonhardt
tleonhardt previously approved these changes May 2, 2018
":python_version<'3.5'": ['contextlib2', 'typing'],
}

if int(setuptools.__version__.split('.')[0]) < 18:
Copy link
Member

@tleonhardt tleonhardt May 2, 2018

Choose a reason for hiding this comment

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

@kotfu
setuptools 18 dates back to early/mid 2015, so I don't think we really need to worry about people using a version prior to this. If you think I'm off-base, please feel free to revert this.

I probably should have put this on master, but you made some changes to this file as well so I didn't want to create any conflicts.

platforms=['any'],
packages=['cmd2'],
keywords='command prompt console cmd',
python_requires='>=3.4',
Copy link
Member

Choose a reason for hiding this comment

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

I figure we need some way of ensuring nobody tries to install the new version of cmd2 on older un-supported versions of Python. If we can make the assumption that people will have a pretty new version of setuptools >= 34.4 then this is a really easy way. If we think it isn't wise to make that assumption, then we could remove this and do something more like:

import sys
if sys.version_info < (3,4):
    sys.exit('Sorry, Python < 3.4 is not supported by this version of cmd2.  Install cmd2 0.8.x instead')

@tleonhardt
Copy link
Member

@kotfu
@anselor
I think we are about to be in for some merge pain in the near future do to multiple large incoming PRs.

Would you be OK with deleting the TODO file in this PR, opening a new Issue for the refactoring that should be done in relation to this PR, and then merging this PR in as-is?

I think that might be the least painful path forward.

@anselor
Copy link
Contributor

anselor commented May 2, 2018

I tested merges between bash_completion and pyscript as well as between ply and pyscript and both resulted in very minor merge conflicts that were easy to resolve.

The main conflict is between the bash_completion and ply branches because I got rid of imports in __init__.py to improve response time in bash completion. Removing those imports caused changes in imports in all of the tests and examples that are also changed in the ply branch.

The bulk of the changes in both the bash_completion and pyscript branches are in new files. We can probably opt to favor the changes in the ply branch for most of the conflicting changes in the examples and tests.

@kotfu
@tleonhardt

@tleonhardt
Copy link
Member

@anselor
Since all three PRs are essentially ready to merge (with very minor changes to this one to remove a TODO file), do you have any recommendations regarding merge order of the three that might result in the least pain?

@anselor
Copy link
Contributor

anselor commented May 2, 2018

I tried first merging bash_completion then pyscript and something went wonky and a bunch of unit tests failed.

I tried merging pyscript then bash_completion into master and all of the tests pass.
I then tried merging the result of that into into ply to simulate pulling into ply after the other 2 are merged. There were a few conflicts that were relatively easy to resolve but 31 unit tests are now failing.

@anselor
Copy link
Contributor

anselor commented May 2, 2018

Ok, both bash_completion and pyscript are merged into master. I've also merged the result of that into ply and got the unit tests passing again.

@kotfu please take a look and make sure I didn't jack up your changes.

@kotfu
Copy link
Member Author

kotfu commented May 3, 2018

@anselor Thanks for doing all the hard work to get these three branches to merge. My review and testing of the merged code looks good. I'm going to merge ply into master.

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.

Explore eliminating POSIX_SHLEX and STRIP_QUOTES_FOR_NON_POSIX

5 participants