feat: Add plotly-express JsPlugin implementation and registration #150
Conversation
| with root_provider() as root, (root / "package.json").open("rb") as f: | ||
| package_json = json.load(f) |
There was a problem hiding this comment.
Am I missing something here? You're trying to use root_provider() as a context manager, but it returns a Generator so this is just going to be an error
There was a problem hiding this comment.
So, this code "works" - I don't think it's just an artifact of python duck typing; I think the code is technically correct, but the typing may be more general than we need it to be. I think when I was originally writing this code, I saw a suggestion somewhere that ContextManager should use typing.Generator typing. I'll look to see if there is more explicit typing suggestions for ContextManager...
There was a problem hiding this comment.
At least the "technically correct", which may have lead me or others to using Generator typeing, https://docs.python.org/3.11/library/contextlib.html#contextlib.contextmanager:
The function being decorated must return a generator-iterator when called. This iterator must yield exactly one value, which will be bound to the targets in the with statement’s as clause, if any.
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't know why the official suggestion isn't typing.ContextManager - maybe b/c it was only introduced in 3.6.
There was a problem hiding this comment.
Yea strange, seems like that's what is returned from .as_file and such: https://github.com/python/typeshed/blob/cff2b3db0ca291e153a8bd407f48ac220a381dd8/stdlib/importlib/resources/__init__.pyi#L37
It is returning an AbstractContextManager vs. ContextManager, but seems like those two match anyway (AbstractContextManager, ContextManager)
| package_json = json.load(f) | ||
| return ExpressJsPlugin( | ||
| package_json["name"], | ||
| package_json["version"], |
There was a problem hiding this comment.
Did we discuss getting the version from python instead of from the package.json? So that package.json has a dummy version, and we always pull from the python version so they always match
There was a problem hiding this comment.
I think this is for @jnumainville to discuss and move forward if desired. Also, questions around editable installs and potentially re-pointing the Path for development use-cases.
| @@ -103,20 +103,21 @@ def create_client_connection( | |||
| return figure_connection | |||
|
|
|||
|
|
|||
| class ChartRegistration(Registration): | |||
| class ExpressRegistration(Registration): | |||
There was a problem hiding this comment.
This might be a bigger refactor than originally planned, but I'm not sure this belongs here (nor does the new import create_js_plugin).
The expectation (and documentation) is that users start with:
import deephaven.plot.express as dxbut with code as it stands, dx. will now include ExpressRegistration and create_js_plugin.
As long as the registration is being renamed (an internal type, won't affect any downstream users), I'd suggest it belongs in its own file to avoid this.
There was a problem hiding this comment.
Yeah, fair enough - should be "internal" code.
There was a problem hiding this comment.
I've refactored so JS / registration are internal. It might be reasonable to refacter DeephavenFigureType in the same way.
There was a problem hiding this comment.
Could remove a couple of extra new lines in doc strings, but whatever.
Also wanted to note for posterity we're switching away from __version__ defined in __init__.py as that was a rejected PEP standard: https://peps.python.org/pep-0396/
Now you should use importlib.metadata.version() to get the version from a module, and the core metadata specs are here: https://packaging.python.org/en/latest/specifications/core-metadata/#core-metadata
| class ExpressRegistration(Registration): | ||
| """ | ||
| Register the DeephavenFigureType and a JsPlugin | ||
|
|
| Args: | ||
| callback: Registration.Callback: | ||
| A function to call after registration | ||
|
|
This plumbs automatic JsPlugin registration - the server side depends on deephaven/deephaven-core#4925.