setting runStartupCommands as array

[janosh]

Minor suggestion for improvement: python.dataScience.runStartupCommands must currently be a string. If you use it to run more than one or two commands, the string gets quite long, requiring a lot of horizontal scrolling, making it hard to read and even harder to modify.

How about also allowing that setting to be an array? If an array is detected, simply convert it to string internally:

if (Array.isArray(runStartupCommands)) runStartupCommands = runStartupCommands.join(`\n`)

Then you could have

"python.dataScience.runStartupCommands": [
  "%load_ext autoreload",
  "%autoreload 2",
  "import sys",
  "sys.path.append('${workspaceFolder}')",
  "sys.path.append('${workspaceFolder}/src')",
],

instead of

"python.dataScience.runStartupCommands": "%load_ext autoreload\n%autoreload 2\nimport sys\nsys.path.append('${workspaceFolder}')\nsys.path.append('${workspaceFolder}/src')",

Microsoft Data Science for VS Code Engineering Team: @rchiodo, @IanMatthewHuff, @DavidKutu, @DonJayamanne, @greazer, @joyceerhl

[janosh]

Happy to submit a PR for this. Would it be this line that needs changing?

https://github.com/microsoft/vscode-python/blob/27dfd87f19569451e237760aff2945628d076d3c/src/client/datascience/jupyter/jupyterNotebook.ts#L288-L292

[rchiodo]

Yeah that would be the line where you'd read it. You'd also have to change the type of that value in https://github.com/microsoft/vscode-python/blob/27dfd87f19569451e237760aff2945628d076d3c/src/client/common/types.ts#L392 (maybe to a 'string | string[]') and the definition of the setting in the package.json.

[janosh]

What's the syntax for "type" in package.json? Same as TS?

"python.dataScience.runStartupCommands": {
  "type": "string | string[]",
  // ...
}

[rchiodo]

I don't believe it supports an or condition. You'll have to just change it to array. However the code has to handle old settings so the actual code should be 'string | string[]'

[janosh]

Should I add a test for the array form of runStartupCommands? Doesn't appear to have any tests at all yet.

[rchiodo]

Yeah a unit test for the notebook would probably be good.

[rchiodo]

Or in the notebook.functional.test.ts. Probably easier to just add a test there that sets runStartupCommands and check that they were run.

[janosh]

I'm a little out of my depth with the test suite here. For one thing, trying to setup testing with npm ci is giving me lots of node-gyp errors. I was going to add something like the following to notebook.functional.test.ts. Not sure though how I get the notebook state to assert that a == 1.

runTest('runStartupCommands as array', async () => {
    ioc.getSettings().datascience.runStartupCommands = [
        '%load_ext autoreload',
        '%autoreload 2',
        'import sys',
        "sys.path.append('${workspaceFolder}')",
        'a=1'
    ];
    const exec = ioc.get<IJupyterExecution>(IJupyterExecution);
    await exec.dispose();
});

[joyceerhl]

As far as testing, I'd check if the right versions of everything are installed, based on our CONTRIBUTING.md. Note that node needs to be v12.15. Apologies if you've already seen that!

[rchiodo]

A way to verify that run startup commands in an array works would be to do something like so:

            runTest('Startup commands', async () => {
                    ioc.getSettings().datascience.runStartupCommands = [
                         'b=2'
                         'a=1'
                    ];
                    const notebook= await createNotebook();
                    assert.ok(notebook, 'Server didn't start');

                    await verifySimple(server, `print(b)`, 2);
                    await verifySimple(server, `print(a)`, 1);
                }
            });

[rchiodo]

Sorry you'll also need to add mock values for the different executions (we run our tests without jupyter during PRs)

                addMockData(`b=2\\na=1`, undefined);
                addMockData(`print(b)`, 2);
                addMockData(`print(a)`, 1);

This should allow the test to pass during a PR.

[janosh]

@rchiodo Thanks for helping with the test!

[janosh]

@joyceerhl I did install node@12 before trying npm ci but still ran into this error:

gyp: Call to 'node -e "require('nan')"' returned exit status 0 while in binding.gyp. while trying to load binding.gyp
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1