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

Export to html/pdf/python on Web when connected to remote server #10069

Merged
merged 13 commits into from
May 20, 2022

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented May 19, 2022

Fixes #9996

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for feature-requests.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).

This PR covers following changes:

  • src/platform/progress/decorator.ts and src/platform/progress/progressReporter.ts are now common modules (no node api usage)
  • exportToHTML, exportToPython, exportToPdf, exportToPythonPlain are common modules
  • exportBase has two variations, exportBase.node and exportBase.web. Their major differences are:
    • how they create files exportBase.web doesn't create temp files
    • how they run nbconvert
      • exportBase.node run nbconvert in local process
      • exportBase.web run nbconvert through silent cell execution. It fetches the content through contentsManager and save to workspace fs.
  • fileConverter as the common module contains most logics of how we export files, with some exceptions in node env, which are overridden in fileConverter.node
  • Commands when clauses updated to enable export commands when
    • local env and workspace trusted
    • remote env, and workspace trusted, and document connected to a kernel
  • Tests are updated to adopt the changes

@rebornix rebornix requested a review from a team as a code owner May 19, 2022 18:59
@rebornix rebornix self-assigned this May 19, 2022
Comment on lines +6 to +7
@injectable()
export class ExportToPDF implements INbConvertExport {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like all of your renames are not be detecting as renames? I usually force this with git add -A but not sure if you can do it after the fact.

Copy link
Member Author

Choose a reason for hiding this comment

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

didn't know I could do that, will try with next rename 💪

@inject(IExportDialog) protected readonly filePicker: IExportDialog,
@inject(ExportUtil) protected readonly exportUtil: ExportUtil,
@inject(INotebookImporter) protected readonly importer: INotebookImporter,
@inject(ExportInterpreterFinder) private exportInterpreterFinder: ExportInterpreterFinder
) {}

public async export(
Copy link
Contributor

Choose a reason for hiding this comment

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

is this method needed in these classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I still put it there since we register it as serviceManager.addSingleton<INbConvertExport>(INbConvertExport, ExportBase, 'Export Base');, not sure why it's needed there. I can send a pr later to have it fixed.

import { ReportableAction } from '../progress/types';

@injectable()
export class ExportBase implements INbConvertExport, IExportBase {
Copy link
Contributor

@amunger amunger May 19, 2022

Choose a reason for hiding this comment

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

ExportHelper or Exporter is probably a better name now since they aren't being extended anymore since Base usually implies that it is a parent class

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2022

Codecov Report

Merging #10069 (53d6c57) into main (e6f523d) will decrease coverage by 0%.
The diff coverage is 83%.

@@          Coverage Diff           @@
##            main   #10069   +/-   ##
======================================
- Coverage     64%      64%   -1%     
======================================
  Files        216      216           
  Lines      10060    10095   +35     
  Branches    1613     1618    +5     
======================================
+ Hits        6508     6527   +19     
- Misses      3028     3038   +10     
- Partials     524      530    +6     
Impacted Files Coverage Δ
src/platform/progress/decorator.ts 94% <ø> (ø)
src/platform/export/exportToPDF.ts 66% <66%> (ø)
src/platform/export/exportUtil.ts 74% <74%> (ø)
src/platform/export/fileConverter.ts 77% <77%> (ø)
src/platform/export/exportToPythonPlain.ts 87% <85%> (+3%) ⬆️
src/platform/export/exportUtil.node.ts 73% <86%> (-13%) ⬇️
src/platform/export/exportBase.node.ts 73% <88%> (+7%) ⬆️
src/platform/export/fileConverter.node.ts 95% <92%> (+7%) ⬆️
src/platform/export/exportToHTML.ts 100% <100%> (ø)
src/platform/export/exportToPython.ts 100% <100%> (ø)
... and 9 more

@rebornix rebornix merged commit b6a8567 into main May 20, 2022
@rebornix rebornix deleted the rebornix/exportNbConvertForWeb branch May 20, 2022 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Export in Web
5 participants