Add server side CoreConfig object #6991
Conversation
|
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
| def set_static_dir(self, static_dir): | ||
| self._data['jupyterlab']['staticDir'] = static_dir | ||
|
|
||
| @property |
There was a problem hiding this comment.
doesn't there need to be standard structure here?
jupyterlab/commands.py
Outdated
| """Create a new _AppHandler object | ||
| """ | ||
| self.app_dir = app_dir or get_app_dir() | ||
| self.sys_dir = get_app_dir() | ||
| self.logger = _ensure_logger(logger) | ||
| self.core_data = (core_config or CoreConfig()).data |
There was a problem hiding this comment.
@vidartf I mean that we are using the data here in the app handler.
There was a problem hiding this comment.
Doesn't the docstring of data cover this use?
| @@ -1628,7 +1629,7 @@ def _node_check(logger): | |||
| output = subprocess.check_output([node, 'node-version-check.js'], cwd=HERE) | |||
| logger.debug(output.decode('utf-8')) | |||
| except Exception: | |||
| data = _get_core_data() | |||
| data = CoreConfig().data | |||
| ver = data['engines']['node'] | |||
There was a problem hiding this comment.
For example, we expect there to be 'engines': 'node' metadata.
There was a problem hiding this comment.
The content of data is supposed to be a lab internal detail. As an alternative, I could call the method ._data, to underline the fact? Or ._get_data_internal() ?
There was a problem hiding this comment.
The content of data is supposed to be a lab internal detail
C.f. its docstring.
There was a problem hiding this comment.
Ah, so your custom spin doesn't use our AppHandler, right?
There was a problem hiding this comment.
It does, but it only modifies data via the methods on CoreConfig. As such, the internal detail fields like engine are left untouched.
There was a problem hiding this comment.
I'm on my phone, but doesn't that metadata still need to be there then? As in, we need to document the minimum fields that need to be present?
There was a problem hiding this comment.
Note that the data is originally filled with the data from staging/package.json. This public API only allows for modifying the extensions/mimeExtensions/singletons. The data property (getter only) is meant to be used by whoever consumes the core config (internal only). Technically, since we are returning the inner data by reference, a user could modify the content of data in place. The behavior in this case is undefined (by spec), and can be considered similar in nature to monkey patching. We could try to make this harder to do by accident by doing a deepcopy on access, but I'm not sure if that is worth the hassle.
There was a problem hiding this comment.
It feels off to me that they'd have to use our package
json directly as a starting point. I'd rather they had to provide their own metadata that met a certain spec.
lock
bot
added
the
status:resolved-locked
References
Proposal for fixing #6989
Code changes
Adds a
CoreConfigclass with a few exposed methods, and adds wiring for letting it be threaded through similarly to howapp_diralready is, but without exposing it on the CLI/config interface.Also fixes to deprecation warnings (separate commit).
User-facing changes
None
Backwards-incompatible changes
None.