-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add plotly-express JsPlugin implementation and registration #150
Changes from 4 commits
f7e1109
79143d3
786b378
2609d44
c400a96
2f58734
5c56fe5
ade4c8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
import typing | ||
import pathlib | ||
|
||
import importlib.metadata | ||
import importlib.resources | ||
import json | ||
import sys | ||
from typing import Callable | ||
|
||
from deephaven.plugin.js import JsPlugin | ||
|
||
|
||
class ExpressJsPlugin(JsPlugin): | ||
def __init__( | ||
self, | ||
name: str, | ||
version: str, | ||
main: str, | ||
root_provider: Callable[[], typing.Generator[pathlib.Path, None, None]], | ||
) -> None: | ||
self._name = name | ||
self._version = version | ||
self._main = main | ||
self._root_provider = root_provider | ||
|
||
@property | ||
def name(self) -> str: | ||
return self._name | ||
|
||
@property | ||
def version(self) -> str: | ||
return self._version | ||
|
||
@property | ||
def main(self) -> str: | ||
return self._main | ||
|
||
def distribution_path(self) -> typing.Generator[pathlib.Path, None, None]: | ||
# TODO: Finalize JsPlugin | ||
# https://github.com/deephaven/deephaven-plugin/issues/15 | ||
return self._root_provider() | ||
|
||
|
||
def _create_from_npm_package_json( | ||
root_provider: Callable[[], typing.Generator[pathlib.Path, None, None]] | ||
) -> JsPlugin: | ||
with root_provider() as root, (root / "package.json").open("rb") as f: | ||
package_json = json.load(f) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I missing something here? You're trying to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know why the official suggestion isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea strange, seems like that's what is returned from |
||
return ExpressJsPlugin( | ||
package_json["name"], | ||
package_json["version"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
package_json["main"], | ||
root_provider, | ||
) | ||
|
||
|
||
def _production_root() -> typing.Generator[pathlib.Path, None, None]: | ||
# TODO: Js content should be in same package directory | ||
# https://github.com/deephaven/deephaven-plugins/issues/139 | ||
if sys.version_info < (3, 9): | ||
return importlib.resources.path("js", "plotly-express") | ||
else: | ||
return importlib.resources.as_file( | ||
importlib.resources.files("js").joinpath("plotly-express") | ||
) | ||
|
||
|
||
def _development_root() -> typing.Generator[pathlib.Path, None, None]: | ||
raise NotImplementedError("TODO") | ||
|
||
|
||
def create_js_plugin() -> JsPlugin: | ||
# TODO: Include developer instructions for installing in editable mode | ||
# https://github.com/deephaven/deephaven-plugins/issues/93 | ||
# TBD what editable mode looks like for JsPlugin | ||
return _create_from_npm_package_json(_production_root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
but with code as it stands,
dx.
will now includeExpressRegistration
andcreate_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fair enough - should be "internal" code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored so JS / registration are internal. It might be reasonable to refacter DeephavenFigureType in the same way.