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.

Sign up for GitHub

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

Jump to bottom

Ensure code is executed when last line of selected code is indented #2222

Merged
merged 4 commits into from
Sep 6, 2018
Merged

Ensure code is executed when last line of selected code is indented #2222

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa6c342
Ensure code is executed when last line of selected code is indented
DonJayamanne Jul 21, 2018
b00acb0
Tweak news entry
brettcannon Aug 1, 2018
c4a7d02
Address code review issues
DonJayamanne Aug 2, 2018
3af012e
Fix code review comments
DonJayamanne Sep 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/2167.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure code is executed when the last line of selected code is indented.
10 changes: 8 additions & 2 deletions pythonFiles/normalizeForInterpreter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,15 @@ def normalize_lines(source):

"""
lines = source.splitlines(False)
# If we have two blank lines, then add two blank lines.
# Do not trim the spaces, if we have blank lines with spaces, its possible
# we have indented code.
if (len(lines) > 1 and len(''.join(lines[-2:])) == 0) or \
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this condition if you are checking the other possibilities in the other two conditions?

Copy link
Author

Choose a reason for hiding this comment

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

This is to look for code that looks like the following:

1. if True:
2.     print(1)
3.     # white space
4.     print(3)
  • Assume user selects line 1,2 and 3. Line three has an indentation.
  • If the last line has an indentation, its possible there's more code.

Copy link
Member

Choose a reason for hiding this comment

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

What I'm saying is, isn't (len(lines) > 1 and len(''.join(lines[-2:])) == 0) redundant when also checking source.endswith('\n\n') or source.endswith('\r\n\r\n')?

Copy link
Member

@ericsnowcurrently ericsnowcurrently Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

A line continuation (the trailing \) isn't necessary. Also, the binary operator should be on the beginning of the next line:

if ((...)
     or ...
     or ...
     ):
    ...

Note the parens wrapping the whole expression. Also putting the closing paren on its own line helps with readability.

source.endswith('\n\n') or source.endswith('\r\n\r\n'):
Copy link
Member

Choose a reason for hiding this comment

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

This is just in case the source file has the "other" line ending, right? How important is that case? I was going to point out the possibility of other line endings, but realistically that's not an issue. :)

Copy link
Author

Choose a reason for hiding this comment

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

This is just in case the source file has the "other" line ending, right?

Yes

How important is that case?

Very

ut realistically that's not an issue. :)

Good, lets leave it for now. as no one has reported an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Please elaborate on why handling the "other" line ending matters.

Copy link
Author

Choose a reason for hiding this comment

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

If the file is saved on Windows then line endings will be \r\n, and if opened from a Mac we'll see see \r\n

Copy link
Author

Choose a reason for hiding this comment

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

I.e. cross platform support

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I was thinking about literal strings. However, for those we should be fine. So carry on! :)

Copy link
Member

Choose a reason for hiding this comment

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

Also, this might be less confusing if you put each condition on its own line.

Copy link
Member

Choose a reason for hiding this comment

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

str.endswith() can take a tuple of strings to check:

source.endswith(('\n\n', '\r\n\r\n'))

Copy link
Author

Choose a reason for hiding this comment

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

WOWOWOWOWOWOWOWOWOWOWOW
Ok, I didn't know this was possible and have never seen this before...

trailing_newline = os.linesep * 2
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, is changing it from what it was to the platform-native line separator always the right thing?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is for sending the code to the terminal. We're not modifying the code in the file.

Copy link
Member

Choose a reason for hiding this comment

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

Why would a non-native line ending in a file necessarily be meant to be sent to the terminal as a native line ending? Am I missing some context here?

Copy link
Member

Choose a reason for hiding this comment

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

never mind

# Find out if we have any trailing blank lines
if len(lines[-1].strip()) == 0 or source.endswith('\n'):
trailing_newline = '\n'
elif len(lines[-1].strip()) == 0 or source.endswith('\n') or source.endswith('\r\n'):
Copy link
Member

Choose a reason for hiding this comment

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

As above, you don't need that first condition, right?

Copy link
Author

Choose a reason for hiding this comment

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

yes we do

Copy link
Member

Choose a reason for hiding this comment

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

Please explain why.

Copy link
Member

Choose a reason for hiding this comment

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

As above, pass a tuple of possible endings to source.endswith().

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

trailing_newline = os.linesep
else:
trailing_newline = ''

Copy link
Member

Choose a reason for hiding this comment

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

There's a spot further down that uses \n instead of os.linesep (line 114 of the new file, sys.stdout.write('\n'.join(lines) + trailing_newline)). If your intention is to always use the native line separator then this needs to change.

Expand Down
15 changes: 10 additions & 5 deletions src/test/terminals/codeExecution/helper.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,16 @@ suite('Terminal - Code Execution Helper', () => {
const expectedCode = await fs.readFile(path.join(TEST_FILES_PATH, `sample${fileNameSuffix}_normalized.py`), 'utf8');
await ensureBlankLinesAreRemoved(code, expectedCode);
});
// test(`Ensure blank lines are removed, including leading empty lines (${fileName})`, async () => {
// const code = await fs.readFile(path.join(TEST_FILES_PATH, `${fileName}_raw.py`), 'utf8');
// const expectedCode = await fs.readFile(path.join(TEST_FILES_PATH, `${fileName}_normalized.py`), 'utf8');
// await ensureBlankLinesAreRemoved(['', '', ''].join(EOL) + EOL + code, expectedCode);
// });
test(`Ensure last two blank lines are preserved (Sample${fileNameSuffix})`, async () => {
const code = await fs.readFile(path.join(TEST_FILES_PATH, `sample${fileNameSuffix}_raw.py`), 'utf8');
const expectedCode = await fs.readFile(path.join(TEST_FILES_PATH, `sample${fileNameSuffix}_normalized.py`), 'utf8');
await ensureBlankLinesAreRemoved(code + EOL, expectedCode + EOL);
});
test(`Ensure last two blank lines are preserved even if we have more than 2 trailing blank lines (Sample${fileNameSuffix})`, async () => {
const code = await fs.readFile(path.join(TEST_FILES_PATH, `sample${fileNameSuffix}_raw.py`), 'utf8');
const expectedCode = await fs.readFile(path.join(TEST_FILES_PATH, `sample${fileNameSuffix}_normalized.py`), 'utf8');
await ensureBlankLinesAreRemoved(code + EOL + EOL + EOL + EOL, expectedCode + EOL);
});
});
test('Display message if there\s no active file', async () => {
documentManager.setup(doc => doc.activeTextEditor).returns(() => undefined);
Expand Down
13 changes: 4 additions & 9 deletions ...ls/codeExecution/terminalCodeExec.test.ts → ...deExecution/terminalCodeExec.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

// tslint:disable:no-multiline-string no-trailing-whitespace
// tslint:disable:no-multiline-string no-trailing-whitespace max-func-body-length

import { expect } from 'chai';
import * as path from 'path';
import * as TypeMoq from 'typemoq';
import { Disposable, Uri, WorkspaceFolder } from 'vscode';
import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../../client/common/application/types';
import { noop } from '../../../client/common/core.utils';
import { IFileSystem, IPlatformService } from '../../../client/common/platform/types';
import { ITerminalService, ITerminalServiceFactory } from '../../../client/common/terminal/types';
import { IConfigurationService, IPythonSettings, ITerminalSettings } from '../../../client/common/types';
Expand All @@ -17,9 +18,7 @@ import { TerminalCodeExecutionProvider } from '../../../client/terminals/codeExe
import { ICodeExecutionService } from '../../../client/terminals/types';
import { PYTHON_PATH } from '../../common';

// tslint:disable-next-line:max-func-body-length
suite('Terminal - Code Execution', () => {
// tslint:disable-next-line:max-func-body-length
['Terminal Execution', 'Repl Execution', 'Django Execution'].forEach(testSuiteName => {
let terminalSettings: TypeMoq.IMock<ITerminalSettings>;
let terminalService: TypeMoq.IMock<ITerminalService>;
Expand All @@ -46,7 +45,6 @@ suite('Terminal - Code Execution', () => {
disposables = [];
});

// tslint:disable-next-line:max-func-body-length
setup(() => {
terminalFactory = TypeMoq.Mock.ofType<ITerminalServiceFactory>();
terminalSettings = TypeMoq.Mock.ofType<ITerminalSettings>();
Expand Down Expand Up @@ -76,8 +74,7 @@ suite('Terminal - Code Execution', () => {
case 'Django Execution': {
isDjangoRepl = true;
workspace.setup(w => w.onDidChangeWorkspaceFolders(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => {
// tslint:disable-next-line:no-empty
return { dispose: () => { } };
return { dispose: noop };
});
executor = new DjangoShellCodeExecutionProvider(terminalFactory.object, configService.object, workspace.object, documentManager.object,
platform.object, commandManager.object, fileSystem.object, disposables);
Expand Down Expand Up @@ -119,7 +116,6 @@ suite('Terminal - Code Execution', () => {
});
});

// tslint:disable-next-line:max-func-body-length
suite(testSuiteName, () => {
setup(() => {
terminalFactory.setup(f => f.getTerminalService(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => terminalService.object);
Expand Down Expand Up @@ -323,8 +319,7 @@ suite('Terminal - Code Execution', () => {
terminalService.setup(t => t.onDidCloseTerminal(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((callback => {
closeTerminalCallback = callback;
return {
// tslint:disable-next-line:no-empty
dispose: () => void 0
dispose: noop
};
}));

Expand Down