refactoring the PubSub class

[szelga]

a possibility of using Ultra JSON encoder/decoder would be cool. what else apart from https://github.com/moccu/django-omnibus/blob/master/omnibus/pubsub.py#L104 and settings should be changed?

[stephrdev]

Do you have any idea how to make the json encoder/decoder exchangeable? I'm open to have it swappable.

[szelga]

i've thought of 2 variants:

1. OMNIBUS_USE_UJSON config option. if ujson can't be imported, fall back to json.
2. more flexible OMNIBUS_JSON_ENCODER. could be set to "json" (default), "ujson" or some other module which has proper dumps function. also, we should have OMNIBUS_JSON_ENCODER_KWARGS option, because ujson's options are different from standard json's. this option would be great in the variant 1 too.

[stephrdev]

To be honest, I don't like to many settings. Maybe, we could make the pubsub class exchangable at all? This would allow developers to rewrite any part of it, if needed.

[szelga]

json.dumps is also used in connection.py which is not that important though, because "internal" MessageConnection messages are pretty lightweight.

I don't like to many settings

so OMNIBUS_PUBSUB_CLASS setting then?

[EnTeQuAk]

Hey @szelga what do you need ujson for, only for performance reasons? You should be able to swap the pubsub class via factory-settings and hook-in your own subclass.

See OMNIBUS_CONNECTION_FACTORY setting (http://django-omnibus.readthedocs.org/en/latest/server/configuration.html#omnibus-connection-factory) and the code in omnibus.factories

[szelga]

i won't be able to swap pubsub class via factory-settings (at least, not in an elegant way) because PubSub instance is given to webapp factory in omnibus/management/commands/omnibusd.py.

so i have 3 suggestions to improve the expandability of pubsub stuff:

1. PubSub class setting.
2. refactor the PubSub class so it would be easier to extend. other than ujson, one could make PubSub class based on redis pubsub functionality, for instance.
3. the same pubsub instance in omnibus/api.py and omnibus/management/commands/omnibusd.py, maybe? (it kinda makes sense but i don't know if it would be significant.)

[szelga]

what PubSub methods apart from publish, subscribe and unsubscribe are "exposed to public"?

[EnTeQuAk]

I don't quite see why you can't swap the class, the factory is being queried from settings (line 29ff)

        # Get factories for connection and tornado webapp.
        authenticator_factory = import_by_path(AUTHENTICATOR_FACTORY)
        connection_factory = import_by_path(CONNECTION_FACTORY)
        webapp_factory = import_by_path(WEBAPP_FACTORY)

thus you should be able to swap in your own factory and use your PubSub subclass there.

[EnTeQuAk]

besides that @szelga are all methods exposed to the public but usually you only use publish subscribe or unsubscribe but nothing is preventing you from using the connection manually if you need to.

[szelga]

thus you should be able to swap in your own factory and use your PubSub subclass there.

well, i guess, i could just ignore pubsub_instance in the custom connection_factory and create my own. also, i need to monkey-patch omnibus.api, so all in all this wouldn't be a neat piece of code.

[EnTeQuAk]

@szelga I'm refactoring the pubsub-code right now to move the initialization to a separate factory that can be replaced. Hope this simplifies how you use it. Stay tuned :smiley:

[szelga]

cool, thanks!

[EnTeQuAk]

https://github.com/moccu/django-omnibus/commit/838b4a19e7366d6694f307ebd78b141f8334697a and other commits in this branch. Let me know what you think. Will be merged soon.

[EnTeQuAk]

Still WIP though, this is only a draft. Need to cleanup some code first and make the tests run properly.