Pull requests

#21 Declined
Repository
elarson elarson
Branch
default
Repository
cherrypy cherrypy
Branch
default

Add a signals plugin to wrap the console/signal handlers for use outside of quickstart.

Author
  1. Eric Larson
Reviewers
Description

After some feedback, here is a better solution.

I did try adding handle_signals to the Bus class but that ended up creating the situation where it was expecting certain plugins to exist under specific names. That seemed like a bad dependency. While I still think cherrypy.engine.handle_signals() is the better UI, cherrypy.engine.signals.subscribe() is reasonably clear as well.

Comments (5)

  1. Robert Brewer

    A little of both. I rather expected it to be a new method on the Bus class. But the other way would be cherrypy.handle_signals() instead of engine.handle_signals(). There's no reason to monkey patch.

  2. Eric Larson author

    I agree it is better to add it to the bus. It makes for a better user experience:

    cherrypy.engine.handle_signals()
    cherrypy.engine.start()
    cherrypy.engine.block
    

    I'll submit a new patch.

  3. Jason R. Coombs

    I suggest just merging 473f9a0 and discard the other approach.

    Should this be included in 3.2.x as well? If so, the changeset should be grafted there, and then merged into default (not available in web UI unfortunately).