Skip to content

Fix markdown cell marker formatting for export to Python #14360

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

Merged
merged 4 commits into from
Oct 12, 2020
Merged

Conversation

joyceerhl
Copy link

For https://github.com/microsoft/vscode-python/issues/14359

We changed how we format nbconvertBaseTemplateFormat, the template itself also needed updating:

{% block markdowncell scoped %}{0} [markdown]

and

this.nbconvertBaseTemplateFormat.format(
nbconvert6 ? this.nbconvert6Null : this.nbconvert5Null,
this.defaultCellMarker

  • 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 enhancements.
  • 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).
  • The wiki is updated with any design decisions/details.

@joyceerhl
Copy link
Author

Our tests didn't catch this because the export/import test doesn't verify that the contents of a notebook explicitly match some expected string; it only verifies that the cell markers and chdir code are present, and the result of the import is totally stubbed out.

I realized this after I wrote a test to defend against this specific bug and found that notebook.functional.test.ts currently uses a MockPythonService and MockProcessService for the Python daemon, so daemon.execModule returns dummy data that is set up in mockJupyterManager.ts, and doesn't actually evaluate whether the conversion works.

I'd really like to run the following with a real Python interpreter--do we already do that anywhere in our testing infrastructure?

            runTest('Export simple notebook to Python file', async () => {
                const notebookJson = `{
                    "metadata": {
                     "language_info": {
                      "codemirror_mode": {
                       "name": "ipython",
                       "version": 3
                      }
                     },
                     "orig_nbformat": 4,
                     "kernelspec": {
                      "name": "JUNK",
                      "display_name": "JUNK"
                     }
                    },
                    "nbformat": 4,
                    "nbformat_minor": 2,
                    "cells": [
                     {
                      "source": [
                       "# This is markdown"
                      ],
                      "cell_type": "markdown",
                      "metadata": {}
                     },
                     {
                      "cell_type": "code",
                      "execution_count": 1,
                      "metadata": {},
                      "outputs": [
                       {
                        "output_type": "stream",
                        "name": "stdout",
                        "text": [
                         "This is code\n"
                        ]
                       }
                      ],
                      "source": [
                       "print('This is code')"
                      ]
                     }
                    ]
                   }`;
                const expectedOutput =
                    "# To add a new cell, type '# %%'\n# To add a new markdown cell, type '# %% [markdown]'\n# %% [markdown]\n# # This is markdown\n\n# %%\nprint('This is code')\n";
                const exportInterpreterFinder = ioc.serviceManager.get<ExportInterpreterFinder>(
                    ExportInterpreterFinder
                );
                const fileSystem = ioc.serviceManager.get<IDataScienceFileSystem>(IDataScienceFileSystem);
                const temp = await fileSystem.createTemporaryLocalFile('.ipynb');
                await fileSystem.writeLocalFile(temp.filePath, notebookJson);
                const usable = await exportInterpreterFinder.getExportInterpreter(ExportFormat.python, undefined);
                assert.isDefined(usable);
                const importer = ioc.serviceManager.get<INotebookImporter>(INotebookImporter);
                const results = await importer.importFromFile(Uri.file(temp.filePath), usable!);
                assert.include(results, expectedOutput);
            });

@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #14360 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #14360   +/-   ##
=======================================
  Coverage   59.44%   59.44%           
=======================================
  Files         716      716           
  Lines       39953    39953           
  Branches     5789     5789           
=======================================
  Hits        23750    23750           
  Misses      14946    14946           
  Partials     1257     1257           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74253ef...4d72479. Read the comment docs.

@@ -36,7 +36,7 @@ export class JupyterImporter implements INotebookImporter {
{% endblock codecell %}
{% block in_prompt %}{% endblock in_prompt %}
{% block input %}{{ cell.source | ipython2python }}{% endblock input %}
{% block markdowncell scoped %}{0} [markdown]
{% block markdowncell scoped %}{1} [markdown]
Copy link
Member

Choose a reason for hiding this comment

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

Ahhh, good catch. I think that I had that change with my convert fixes, but accidentally reverted it before checkin. Thanks for the fix.

@IanMatthewHuff
Copy link
Member

@joyceerhl These same tests run with real interpreters as well in our nightly flake tests, just depends on the ENV vars that are set. Thanks to Rich's recent changes they will even run on real interpreters on PR submission in vscode-jupyter.

@joyceerhl
Copy link
Author

@IanMatthewHuff, got it, I should've noticed that we do run with real Python. In that case I just added an extra line to the test to guard against this particular case.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@joyceerhl joyceerhl merged commit 5772a90 into main Oct 12, 2020
@joyceerhl joyceerhl deleted the python-export branch October 12, 2020 19:57
luabud pushed a commit to luabud/vscode-python that referenced this pull request Oct 26, 2020
DonJayamanne pushed a commit that referenced this pull request Oct 30, 2020
…-jupyter (#194)

* Fix sample notebook to state that you can double click to edit a cell (#14238)

* Disable surveys if running in codespaces (#14332)

* Disable surveys if running in codespaces

* Disable gather survey in codespaces too

* For gather, show a description without the survey

* Fixes for IW tests where UIKind is not defined

* Fix all the tests where UIKind is not defined

* Appease hygiene

* Disable mailing list if running in Codespaces (#14347)

* Show tensor dimensions in variable explorer (#14244)

* Fix markdown cell marker formatting for export to Python (#14360)
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.

4 participants