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

adding nbconvert to services and using to load exporter list from nbconvert #5217

Merged
merged 9 commits into from Sep 6, 2018

Conversation

@dharmaquark
Copy link
Contributor

@dharmaquark dharmaquark commented Aug 25, 2018

first pass at changes for #5015
I've added nbconvert to services, and hooked this up in notebook-extensions to start (going to making/testing equivalent change in the cell inspector (for setting the drop down items in the 'Raw NBConvert Format' combo box)

a couple of things that need polishing during this review:

  • should I add any type checking or formatting in NbconvertManager's getExportFormats? Right now, I'm just passing through the object that the api/nbonvert root endpoint returns.
  • I'm leaving in an object to map the exporter key/name to the label that shows up in the command/menu item (wasn't sure if that should live in a more shared utility or in the nbconvert services logic?)
  • I've added a 'black list' of sorts to the notebook-extension logic to prevent notebook/python/custom from showing up as export options, not sure if that's the cleanest way to filter out these options. For reference, this is the current default exporter list that gets returned by the endpoint:

{
"custom": {
"output_mimetype": ""
},
"html": {
"output_mimetype": "text/html"
},
"slides": {
"output_mimetype": "text/html"
},
"latex": {
"output_mimetype": "text/latex"
},
"pdf": {
"output_mimetype": "text/latex"
},
"markdown": {
"output_mimetype": "text/markdown"
},
"python": {
"output_mimetype": "text/x-python"
},
"rst": {
"output_mimetype": "text/restructuredtext"
},
"notebook": {
"output_mimetype": "application/json"
},
"script": {
"output_mimetype": ""
}
}

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Aug 25, 2018

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks @dharmaquark, this looks great! The decisions you made in your bullet list seem good to me.

Eventually we could consider moving the calling of nbconvert to your NbconvertManager class as well, instead of where it is in notebook-extension:

const url =
URLExt.join(
services.serverSettings.baseUrl,
'nbconvert',
args['format'] as string,
notebookPath
) + '?download=true';
const child = window.open('', '_blank');
const { context } = current;

That can be a follow-up, however.

const NBCONVERT_SETTINGS_URL = 'api/nbconvert';

/**
* The static namespace for `NbconvertManager`.
Copy link
Member

@ian-r-rose ian-r-rose Aug 26, 2018

Choose a reason for hiding this comment

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

This comment seems misapplied.

/**
* The static namespace for `NbconvertManager`.
*/
export class NbconvertManager {
Copy link
Member

@ian-r-rose ian-r-rose Aug 26, 2018

Choose a reason for hiding this comment

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

A small nit, but to me a capital case name for this would be NbConvertManger, and NbConvert below.

/**
* Build the application.
*/
build(): Promise<void> {
Copy link
Member

@ian-r-rose ian-r-rose Aug 26, 2018

Choose a reason for hiding this comment

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

build and cancel here and below seem like a copy-paste error?

}

/**
* The build status response from the server.
Copy link
Member

@ian-r-rose ian-r-rose Aug 26, 2018

Choose a reason for hiding this comment

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

Out-of-date comment.

/**
* The build status response from the server.
*/
export interface IExportFormats {
Copy link
Member

@ian-r-rose ian-r-rose Aug 26, 2018

Choose a reason for hiding this comment

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

It seems to me like this should be typed something like

export interface IExportFormats {
  [key: string]: { output_mimetype: string }
}

That is the format of the JSON response, and it is how you use it in notebook-extension.

return response.json();
})
.then(data => {
/* TODO: should we add a type check here?
Copy link
Member

@ian-r-rose ian-r-rose Aug 26, 2018

Choose a reason for hiding this comment

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

It could be checked, or transformed to a simpler object like

{
  [key: string]: string
}

using a mapping operation:

return Object.keys(data).map(format => ({ format: data[format].output_mimetype }));

I think it's okay as is, as well, as long as the type definition below matches it.

@bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Aug 27, 2018

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Aug 27, 2018

@bollwyvl Agreed, those display labels should probably be added upstream.

@starcruiseromega
Copy link

@starcruiseromega starcruiseromega commented Aug 27, 2018

@bollwyvl @ian-r-rose It looks like there's already sort of a label like this: see jupyter/nbconvert#864 and jupyter/notebook#3879 for the changes I have to use the existing entrypoint registration for this endpoint. It's just a single string though, so I'm not sure that's the best solution from a perspective of localization and stuff like that but if you want to comment there I can make updates to those PRs.

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Aug 27, 2018

thanks for the review! I cleaned up the comments/removed the copied relics from the build service
still wrangling with typescript for adding the mapping operation in getExportFormats (getting an 'index signature is missing in type' error when I try to return something like return Object.keys(data).map(format => ({ format: data[format].output_mimetype })); - thinking I have to explicitly flag 'format' and 'data[format].output_mimetype' as strings here for that to work, but I'd like to add that in to be more explicit/type safe about what this function returns)

*/
export interface IExportFormats {
/**
* The list of supported export formats.
*/
// TODO: should this stay a string, or a typed object
// that includes an 'output_mimetype' string?
readonly exportList: string;
[key: string]: { output_mimetype: string };
Copy link
Contributor Author

@dharmaquark dharmaquark Aug 27, 2018

Choose a reason for hiding this comment

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

going to leave the output_mimetype element (versus the simpler string->string map), in the event that with @starcruiseromega's changes we can add another 'label' type piece of metadata to this response

Copy link
Member

@ian-r-rose ian-r-rose Sep 5, 2018

Choose a reason for hiding this comment

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

Sounds good to me.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

This looks great. My only suggestion is that it might be a touch simpler to combine the two calls to getExportFormats() into one, which adds the items to both the command palette and the menu. Not a big deal either way, though.

*/
export interface IExportFormats {
/**
* The list of supported export formats.
*/
// TODO: should this stay a string, or a typed object
// that includes an 'output_mimetype' string?
readonly exportList: string;
[key: string]: { output_mimetype: string };
Copy link
Member

@ian-r-rose ian-r-rose Sep 5, 2018

Choose a reason for hiding this comment

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

Sounds good to me.

/**
* The list of supported export formats.
*/
// TODO: should this stay a string, or a typed object
Copy link
Member

@ian-r-rose ian-r-rose Sep 5, 2018

Choose a reason for hiding this comment

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

Stale TODO.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Sep 5, 2018

I think it would also be a good idea to take @bollwyvl 's suggestion and Capital Case the labels of exporters that are not in the default list, so that unknown ones have a better chance of showing up nicely in the menu/palette items.

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 5, 2018

thanks! yup, forgot to add the Capital Case change, I'll add that in, and take care of the stale TODO, and combine the getExportFormats calls! I'll also add this change to the cell inspector logic!

going to put in the change to move the calling of nbconvert to the NbconvertManager class (instead of where it is in notebook-extension) as a separate, followup PR, if that's ok!

@blink1073 blink1073 removed this from the 0.35 milestone Sep 5, 2018
@blink1073 blink1073 added this to the 1.0 milestone Sep 5, 2018
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Sep 5, 2018

Sounds great, thanks @dharmaquark

* The exluded Cell Inspector Raw NbConvert Formats
* (returned from nbconvert's export list)
*/
const RAW_FORMAT_EXCLUDE = ['pdf', 'slides', 'script', 'notebook', 'custom'];
Copy link
Contributor Author

@dharmaquark dharmaquark Sep 5, 2018

Choose a reason for hiding this comment

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

added a second 'blacklist' for the cell inspector raw nbconvert options, which may be a little clunky- happy to clean this up if there are other alternatives for filtering out format types! wasn't sure if it made sense to have this kind of filtering logic live in notebook-extension, or in the nbconvert services logic? or if long-term the export/filter options should/would be set at the nb server level?

Copy link
Member

@ian-r-rose ian-r-rose Sep 6, 2018

Choose a reason for hiding this comment

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

I think this is fine in the notebook extension. If we want to go back and make it more flexible/configurable later, we can do that. But I think this is a really nice improvement already.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Just one minor request for the cell tools panel, and then I think we should merge.

packages/notebook-extension/src/index.ts Show resolved Hide resolved
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Sep 6, 2018

Thanks @dharmaquark, this is really great.

@ian-r-rose ian-r-rose merged commit d18a539 into jupyterlab:master Sep 6, 2018
2 checks passed
@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 6, 2018

thanks!!

@blink1073 blink1073 removed this from the 1.0 milestone Sep 28, 2018
@blink1073 blink1073 added this to the 0.35 milestone Sep 28, 2018
@blink1073 blink1073 mentioned this pull request Sep 28, 2018
31 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 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

5 participants