Skip to content
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

Add server side CoreConfig object #6991

Merged
merged 11 commits into from Aug 14, 2019
Merged

Add server side CoreConfig object #6991

merged 11 commits into from Aug 14, 2019

Conversation

@vidartf
Copy link
Member

@vidartf vidartf commented Aug 12, 2019

References

Proposal for fixing #6989

Code changes

Adds a CoreConfig class with a few exposed methods, and adds wiring for letting it be threaded through similarly to how app_dir already 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.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Aug 12, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@vidartf vidartf changed the title Coreconfig Add server side CoreConfig object Aug 12, 2019
Copy link
Member

@blink1073 blink1073 left a comment

Looks great! Just a few comments.

jupyterlab/coreconfig.py Outdated Show resolved Hide resolved
jupyterlab/coreconfig.py Outdated Show resolved Hide resolved
def set_static_dir(self, static_dir):
self._data['jupyterlab']['staticDir'] = static_dir

@property
Copy link
Member

@blink1073 blink1073 Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't there need to be standard structure here?

Copy link
Member Author

@vidartf vidartf Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean

"""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
Copy link
Member

@blink1073 blink1073 Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vidartf I mean that we are using the data here in the app handler.

Copy link
Member Author

@vidartf vidartf Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']
Copy link
Member

@blink1073 blink1073 Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, we expect there to be 'engines': 'node' metadata.

Copy link
Member Author

@vidartf vidartf Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() ?

Copy link
Member Author

@vidartf vidartf Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content of data is supposed to be a lab internal detail

C.f. its docstring.

Copy link
Member

@blink1073 blink1073 Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so your custom spin doesn't use our AppHandler, right?

Copy link
Member Author

@vidartf vidartf Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but it only modifies data via the methods on CoreConfig. As such, the internal detail fields like engine are left untouched.

Copy link
Member

@blink1073 blink1073 Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@vidartf vidartf Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@blink1073 blink1073 Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@blink1073 blink1073 left a comment

LGTM, thanks!

@blink1073 blink1073 merged commit 860877e into jupyterlab:master Aug 14, 2019
9 checks passed
@vidartf vidartf deleted the coreconfig branch Aug 14, 2019
@blink1073 blink1073 added this to the 1.1 milestone Aug 22, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants