Skip to content

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Nov 8, 2018

For #3249 - Run all cells with markdown makes markdown come out first

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

I think I found a bug in line 151 (https://github.com/Microsoft/vscode-python/blob/9525fd74490841f7f105e198c2e789eac9681cea/src/client/datascience/jupyterServer.ts#L151)

    public execute(code : string, file: string, line: number) : Promise<ICell[]> {
        // Create a deferred that we'll fire when we're done
        const deferred = createDeferred<ICell[]>();

        // Attempt to evaluate this cell in the jupyter notebook
        const observable = this.executeObservable(code, file, line);
        let output: ICell[];

        observable.subscribe(
            (cells: ICell[]) => {
                output = cells;
            },
            (error) => {
                deferred.resolve(output);
            },
            () => {
                deferred.resolve(output);
            });

        // Wait for the execution to finish
        return deferred.promise;
    }

Shouldn't it be deferred.reject(error);

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

You might want to change the following code:

            // Replace windows line endings with unix line endings.
            const copy = code.replace(/\r\n/g, '\n');

            // Determine if we have a markdown cell/ markdown and code cell combined/ or just a code cell
            const split = copy.split('\n');

to just (making it unnecessary to replace CRLF):

const split = copy.splitlines();

@rchiodo
Copy link
Author

rchiodo commented Nov 8, 2018

The copy.splitlines doesn't buy me anything because I still need the copy by itself. #Resolved

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Please create a GitHub issue to create functional tests to ensure we cover this scenario.

@DonJayamanne
Copy link

@rchiodo Please do remember to create functional tests to cover this scenario (to ensure this doesn't come up again).
FYI - we don't have any tests to cover this today (neither functional nor unit tests).

@rchiodo
Copy link
Author

rchiodo commented Nov 8, 2018

Yep just did: #3257

@rchiodo
Copy link
Author

rchiodo commented Nov 8, 2018

My next task is to add a bunch of functional tests for the history window. I should be able to test this scenario there.

@rchiodo rchiodo merged commit 68cfb2c into master Nov 8, 2018
@rchiodo rchiodo deleted the rchiodo/fix_markdown_out_of_order branch November 8, 2018 19:08
@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants