Skip to content

Conversation

@kotfu
Copy link
Member

@kotfu kotfu commented Jul 8, 2018

Add methods which allow multiple application and command processing lifecycle hooks to be registered. All registered hooks are called at the appropriate point of the application or command processing lifecycle.

The primary driver for this change is better support for plugins. Prior to this PR, if a plugin implements an application or command processing hook, the plugin will break if the cmd2.Cmd application also implements the same hook.

This PR deprecates all of the current application and command processing lifecycle hooks, except those required to retain backwards compatibility with cmd.Cmd. Complete documentation of the new system, including the now deprecated methods can be found in hooks.rst.

I have created a new plugin which implements the recently removed abbreviation behavior. The plugin can be found at https://github.com/python-cmd2/cmd2-abbrev. The plugin utilizes the hook methods in this PR.

I have also created a plugin template project (https://github.com/python-cmd2/cmd2-plugin-template) which implements a sample plugin and includes best practices and recommendations for plugin developers.

TODO:

  • Make sure preparse gets called for backwards compatibility reasons
  • Also update documentation in hooks.rst to show when this gets called
  • Consider adding a list of hooks analogous to preparse
  • Updated documentation in integrating.rst

kotfu added 30 commits May 26, 2018 14:01
# Conflicts:
#	cmd2/cmd2.py
tleonhardt
tleonhardt previously approved these changes Jul 16, 2018
""".splitlines())))

INSTALL_REQUIRES = ['pyperclip >= 1.5.27', 'colorama']
INSTALL_REQUIRES = ['pyperclip >= 1.5.27', 'colorama', 'attrs']
Copy link
Member

Choose a reason for hiding this comment

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

Anytime we add a dependency we should updated the README.md accordingly since we have a section which discusses the required dependencies.

parser = StatementParser()
return parser

def test_parse_empty_string_default(default_parser):
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the extra unit tests

backwards compatibility with the standard library version of cmd.
"""
statement = self.statement_parser.parse(line)
statement = self.statement_parser.parse(self.preparse(line))
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for making sure preparse now properly gets called

"""Hook method executed just before the command line is interpreted, but after the input prompt is generated.
"""Hook method executed before user input is parsed.
WARNING: If it's a multiline command, `preparse()` may not get all the
Copy link
Member

Choose a reason for hiding this comment

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

Good comment

@tleonhardt
Copy link
Member

@kotfu
Before merging, you should update the README to mention the new dependency on attrs and the CHANGELOG to mention the overall changes.

@kmvanbrunt I know you are pretty busy at work, but if you get a chance, I would value your feedback for this PR.

Also:
- Bumped version to 0.9.4
- Updated info in Readme and Sphinx docs to reflect new dependency on attrs
@tleonhardt
Copy link
Member

I stared updating the CHANGELOG for you and bumped the version to 0.9.4.

I also updated the README and Sphinx docs to reflect the new dependency on attrs.

tleonhardt
tleonhardt previously approved these changes Jul 17, 2018
## 0.9.4 (TBD, 2018)
* Bug Fixes
* Fixed bug where ``preparse`` wasn't getting called
* Enhancements
Copy link
Member

Choose a reason for hiding this comment

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

Please fill in a better description of your changes

tleonhardt
tleonhardt previously approved these changes Jul 17, 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.

This looks good to merge whenever. Lets give it a couple days to see if @kmvanbrunt can find some time to give it a look.

* Fixed bug where ``preparse`` wasn't getting called
* Enhancements
* Improved implementation of lifecycle hooks to to support a plugin
framework, see ``docs/hooks.rst`` for details.
Copy link
Member

Choose a reason for hiding this comment

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

This description looks good. Thanks for updating.

@tleonhardt
Copy link
Member

tleonhardt commented Jul 17, 2018

I manually merged master into this PR because a conflict in test_parsing.py needed to be resolved.

@kotfu @kmvanbrunt
FYI, I am working on incorporating Visual Studio Team Services (VSTS) CI in as well since they are working on free macOS builds for open-source projects. That feature isn't fully implemented yet, so for now I am experimenting using the VSTS integration for it for free Azure-hosted Linux builds.

I will need to add a bug fix or two to this PR to get the VSTS builds passing.

@tleonhardt
Copy link
Member

Sorry for temporarily breaking the build system. I should have it fixed sometime this evening.

Currently this represents VSTS Hosted Linux builds
- But it is a placeholder for eventual VSTS Hosted macOS builds
tleonhardt
tleonhardt previously approved these changes Jul 18, 2018
[![Latest Version](https://img.shields.io/pypi/v/cmd2.svg?style=flat-square&label=latest%20stable%20version)](https://pypi.python.org/pypi/cmd2/)
[![Build status](https://img.shields.io/travis/python-cmd2/cmd2.svg?style=flat-square&label=unix%20build)](https://travis-ci.org/python-cmd2/cmd2)
[![Appveyor build status](https://img.shields.io/appveyor/ci/FedericoCeratto/cmd2.svg?style=flat-square&label=windows%20build)](https://ci.appveyor.com/project/FedericoCeratto/cmd2)
[![VSTS Build status](https://python-cmd2.visualstudio.com/cmd2/_apis/build/status/cmd2-Python%20package-CI?branch=master)](https://python-cmd2.visualstudio.com/cmd2/_build/latest?definitionId=1&branch=master)
Copy link
Member

Choose a reason for hiding this comment

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

The cmd2 PR build/test CI system is now also integrated with Visual Studio Team Services (VSTS). Currently it is doing builds for Python 3.6 and 3.7 on Linux VMs on a Windows host which is redundant with the TravisCI testing. But the idea is that eventually we will use VSTS to do our macOS testing once Microsoft's support for that feature is more complete.

@tleonhardt
Copy link
Member

tleonhardt commented Jul 18, 2018

@kotfu The one thing this PR could really benefit from is a new example called something like hooks.py demonstrating the new methods for registering hooks.

Added typing backport dependency for Python 3.4
VSTS is now running unit tests for Python 3.6.5 on macOS
- In the future we will be able to test on multiple versions of Python on macOS using VSTS, but there is currently a limitation to testing with Python installed from Homebrew
@tleonhardt
Copy link
Member

@kmvanbrunt @anselor
Do either of you have any feedback before this PR gets merged in?

@tleonhardt
Copy link
Member

Ok @kmvanbrunt wouldn’t have time to review this for another week and a half, so I’m just going to merge this PR.

@tleonhardt tleonhardt merged commit 3f737b4 into master Jul 20, 2018
@tleonhardt tleonhardt deleted the plugin_functions branch July 20, 2018 00:32
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.

3 participants