[Issue 243] Move to new metadata spec #254
Conversation
| } | ||
|
|
||
| // get cell view metadata from element data | ||
| function _getCellViewMetadata($elem, viewId) { |
There was a problem hiding this comment.
Maybe _getCellActiveViewMetadata?
There was a problem hiding this comment.
It's not just for the active view. The third argument specifies the view to get and defaults to the active view. See line 145 (_updateCellMetadata calls with 3rd arg) for an example of that arg being used.
There was a problem hiding this comment.
I see it now. Maybe add some doc on it then indicating the default behavior of the param (2nd right?) is missing.
| layout = metadata.layout; | ||
| // common metadata | ||
| _ensureHas(metadata, 'extensions'); | ||
| _ensureHas(metadata.extensions, 'jupyter_dashboards'); |
There was a problem hiding this comment.
Why not update _ensureHas to take the full dot-delimited string, similar to what dojo.getObject does? i.e. _ensureHas(metadata, 'extensions.jupyter_dashboards').
(c) Copyright IBM Corp. 2016
bc3b1ce to
2f04872
Compare
|
Updated based on comments and to add compatibility module. |
| * This must be run AFTER the version 1 metadata is initialized in | ||
| * `dashboard-metadata.js`. | ||
| */ | ||
| convert: function() { |
There was a problem hiding this comment.
If we're going to play the up-convert game, I think we should future proof a bit. Should we make this more amenable to v0 -> v1 conversion explicitly so that later, another convert function / delegate can handle v1 -> v2 and so on?
There was a problem hiding this comment.
Nevermind for now. I see there's an if-statement guarding the convert so that it only runs on v0 (urth) metadata. If a metadata v2 ever happens, older versions of the extension won't try to run their v0->v1 code, and v1->v2 conversion is something we can add to a new version of the extension. For now, YAGNI.
|
@dalogsdon I can't tell because of the squash push, but in your latest commit, did you do anything other than address @jhpedemonte 's comment and add the compatibility support? Need to know how much I have to go back and retest. (Push separate commits if more changes are coming ...) |
|
Updated according to @jhpedemonte's comment about ensuring object exists, according to @parente's comment on the Because the ensure object exist util is used in the generation of the metadata structure, some re-testing may be good to ensure no bugs with the structure. |
|
I'll give it one more round of testing. |
|
Tested all of the recent defects again as well as loading of many existing demo notebooks. No issues found. |
Part of #243