Refactor Hooks/Handlers into CementApp #311

Closed
malinoff opened this Issue Jun 1, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@malinoff

malinoff commented Jun 1, 2015

Every time I instantiate a new cement application, it cleans cement.core.backend.__handlers__ and cement.core.backend.__hooks__. Considering the following (simple!) code:

from cement.core.foundation import CementApp
from cement.core.interface import Interface
from cement.core import handler
from cement.core import backend

class MyInterface(Interface):
    class IMeta:
        label = 'myinterface'

app1 = CementApp('app1')
handler.define(MyInterface)
app2 = CementApp('app2')
print(MyInterface.IMeta.label in backend.__handlers__)

False will be printed. Quite unexpectedly.

If backend.__handlers__ is truly global object, consider to not clean it on every CementApp instantiation. If it is local to specific CementApp object, consider moving from global containers and use app attributes and methods to work with them.

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 1, 2015

Member

This was an early design choice, but was intentional. It obviously did not account for a use case of running multiple unique Cement apps simultaneously.

The reasoning for putting this code in cement.core.backend rather than within the Application object was to keep extensions and core framework code from relying on the App. For example, an application-agnostic framework extension registers its handlers via cement.core.handler.register, registering the handler with the framework, not the application. Putting it in the app would require the extension to be aware of the app... for example from someapp.main import app ... which I did not want at all.

Open to discussing alternative means of course, but the goal of extensions was to keep them application agnostic.

Member

derks commented Jun 1, 2015

This was an early design choice, but was intentional. It obviously did not account for a use case of running multiple unique Cement apps simultaneously.

The reasoning for putting this code in cement.core.backend rather than within the Application object was to keep extensions and core framework code from relying on the App. For example, an application-agnostic framework extension registers its handlers via cement.core.handler.register, registering the handler with the framework, not the application. Putting it in the app would require the extension to be aware of the app... for example from someapp.main import app ... which I did not want at all.

Open to discussing alternative means of course, but the goal of extensions was to keep them application agnostic.

@malinoff

This comment has been minimized.

Show comment
Hide comment
@malinoff

malinoff Jun 1, 2015

@derks thanks for the explanation, but as I can see from foundation.py#L961 and built-in extensions, they all use load function that accepts an application - so in the end they all will be registered by some application instance implicitly.

Actually, this load function is what makes extensions application agnostic, because extensions that register themselves via load function don't know anything about application that invokes the load process. No from someapp.main import app required.

malinoff commented Jun 1, 2015

@derks thanks for the explanation, but as I can see from foundation.py#L961 and built-in extensions, they all use load function that accepts an application - so in the end they all will be registered by some application instance implicitly.

Actually, this load function is what makes extensions application agnostic, because extensions that register themselves via load function don't know anything about application that invokes the load process. No from someapp.main import app required.

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 1, 2015

Member

You are completely right! What was I thinking.... haha. Passing app to the load() function was added pretty recently... and makes complete sense that we can now move the backend stuff into the app object because of it. That was precisely the point of that change...

I will definitely put some time in to testing that out, if all works and no major test breakage should be able to make that into stable/2.8.

Member

derks commented Jun 1, 2015

You are completely right! What was I thinking.... haha. Passing app to the load() function was added pretty recently... and makes complete sense that we can now move the backend stuff into the app object because of it. That was precisely the point of that change...

I will definitely put some time in to testing that out, if all works and no major test breakage should be able to make that into stable/2.8.

@derks derks added this to the 2.7.2 Development milestone Jun 1, 2015

@derks derks self-assigned this Jun 1, 2015

@malinoff

This comment has been minimized.

Show comment
Hide comment
@malinoff

malinoff Jun 1, 2015

Actually, I'm working on refactoring these globals into the app object - I can create a PR if you want.
I'm just worried that this breaks backwards compatibility, and I don't know an easy way to save current behavior.
All I can imagine is that it will be required to inspect the caller's frame and try to find an instance of CementApp (I do this already in refactored foundation:cement_signal_handler, but in this handler the frame is passed explicitly) which still may break things.
What do you think about it? Shouldn't it be in 3.0 (although I'm excited to get this into 2.x)?

malinoff commented Jun 1, 2015

Actually, I'm working on refactoring these globals into the app object - I can create a PR if you want.
I'm just worried that this breaks backwards compatibility, and I don't know an easy way to save current behavior.
All I can imagine is that it will be required to inspect the caller's frame and try to find an instance of CementApp (I do this already in refactored foundation:cement_signal_handler, but in this handler the frame is passed explicitly) which still may break things.
What do you think about it? Shouldn't it be in 3.0 (although I'm excited to get this into 2.x)?

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 1, 2015

Member

Won't know entirely until I really start digging unfortunately. Extensions/Plugins/Etc I think are easy as it would become app.handler.register() rather than handler.register() but there are also pieces in the framework that aren't loaded in that fashion though I think that the majority of framework pieces are already being defined/registered from CementApp._lay_cement() so that isn't too tough. Backward compat for cement.core.backend.* is breakable in 2.8 as it is for internal use, and isn't documented other than a brief mention. It's not likely that too many people are trying to access backend pieces directly.

Long story short... depending on how much breakage there is, yeah it might end up in 3.0, but thankfully I plan on starting on that relatively soon-ish also.

Member

derks commented Jun 1, 2015

Won't know entirely until I really start digging unfortunately. Extensions/Plugins/Etc I think are easy as it would become app.handler.register() rather than handler.register() but there are also pieces in the framework that aren't loaded in that fashion though I think that the majority of framework pieces are already being defined/registered from CementApp._lay_cement() so that isn't too tough. Backward compat for cement.core.backend.* is breakable in 2.8 as it is for internal use, and isn't documented other than a brief mention. It's not likely that too many people are trying to access backend pieces directly.

Long story short... depending on how much breakage there is, yeah it might end up in 3.0, but thankfully I plan on starting on that relatively soon-ish also.

@malinoff

This comment has been minimized.

Show comment
Hide comment
@malinoff

malinoff Jun 1, 2015

Okay, I'll make all tests to pass and create my PR tomorrow.

malinoff commented Jun 1, 2015

Okay, I'll make all tests to pass and create my PR tomorrow.

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 1, 2015

Member

Sorry, yeah the big incompatible piece will be that all applications use the handler and hook libraries directly, without passing the Cement object... which is a pretty significant change, not just a backend/private incompatibility.

Member

derks commented Jun 1, 2015

Sorry, yeah the big incompatible piece will be that all applications use the handler and hook libraries directly, without passing the Cement object... which is a pretty significant change, not just a backend/private incompatibility.

@derks derks added portland and removed stable/2.10.x labels Jun 23, 2015

@derks derks modified the milestones: 3.0.0 Stable, 2.8.0 Stable, Portland Development Jun 23, 2015

@derks derks changed the title from cement.core.backend globals are useless to Refactor Hooks/Handlers into CementApp Jun 23, 2015

derks added a commit that referenced this issue Feb 7, 2016

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Feb 7, 2016

Member

@malinoff I know I've been silent on this for a while, primarily because I felt these changes were too major for Cement 2.x.x. That said, I had an epiphony today on how to implement it quietly without any backward compatibility issues.

I've pushed a new branch at issue311. Would really appreciate it if you might try it out and see if it works for your needs? Essentially, all that is required is to pass use_backend_globals=False to CementApp:

from cement.core.foundation import CementApp
from cement.core.interface import Interface

class MyInterface(Interface):
    class IMeta:
        label = 'myinterface'

# create first app and define handler/hooks
app1 = CementApp('app1', use_backend_globals=False)
app1.handler.define(MyInterface)
app1.hook.define('my_test_hook')


# create a second app
app2 = CementApp('app2', use_backend_globals=False)

# run app1
app1.setup()
app1.run()
app1.log.info("Running App1")
app1.log.info("MyInterface Defined? %s" % app1.handler.defined('myinterface'))

app2.setup()
app2.run()
app2.log.info("Running App2")
app2.log.info("MyInterface Defined? %s" % app2.handler.defined('myinterface'))

Looks like:

$ python myapp.py
INFO: Running App1
INFO: MyInterface Defined? True
INFO: Running App2
INFO: MyInterface Defined? False

Moving forward, I'll be updating all the documentation to reference using CementApp.handler and Cement.hook as the proper way to do things, however anyone using cement.core.handler and cement.core.hook directly will still work.

Cement 3 will drop the deprecated bits and will no longer support the backend globals at all.

Member

derks commented Feb 7, 2016

@malinoff I know I've been silent on this for a while, primarily because I felt these changes were too major for Cement 2.x.x. That said, I had an epiphony today on how to implement it quietly without any backward compatibility issues.

I've pushed a new branch at issue311. Would really appreciate it if you might try it out and see if it works for your needs? Essentially, all that is required is to pass use_backend_globals=False to CementApp:

from cement.core.foundation import CementApp
from cement.core.interface import Interface

class MyInterface(Interface):
    class IMeta:
        label = 'myinterface'

# create first app and define handler/hooks
app1 = CementApp('app1', use_backend_globals=False)
app1.handler.define(MyInterface)
app1.hook.define('my_test_hook')


# create a second app
app2 = CementApp('app2', use_backend_globals=False)

# run app1
app1.setup()
app1.run()
app1.log.info("Running App1")
app1.log.info("MyInterface Defined? %s" % app1.handler.defined('myinterface'))

app2.setup()
app2.run()
app2.log.info("Running App2")
app2.log.info("MyInterface Defined? %s" % app2.handler.defined('myinterface'))

Looks like:

$ python myapp.py
INFO: Running App1
INFO: MyInterface Defined? True
INFO: Running App2
INFO: MyInterface Defined? False

Moving forward, I'll be updating all the documentation to reference using CementApp.handler and Cement.hook as the proper way to do things, however anyone using cement.core.handler and cement.core.hook directly will still work.

Cement 3 will drop the deprecated bits and will no longer support the backend globals at all.

@derks derks added the stable/2.10.x label Feb 7, 2016

@derks derks modified the milestones: 2.8.0 Stable, Portland Development Feb 7, 2016

derks added a commit that referenced this issue Feb 8, 2016

derks added a commit that referenced this issue Feb 8, 2016

@derks derks closed this Feb 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment