Ensure code is executed when last line of selected code is indented by DonJayamanne · Pull Request #2222 · microsoft/vscode-python

DonJayamanne · 2018-07-21T22:50:39Z

Title summarizes what is changing
Includes a news entry file (remember to thank yourself!)
Unit tests & code coverage are not adversely affected (within reason)
Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)
[n/a] Dependencies are pinned (e.g. "1.2.3", not "^1.2.3")
[n/a] package-lock.json has been regenerated if dependencies have changed

codecov · 2018-07-21T23:16:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6af8f99). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2222   +/-   ##
=========================================
  Coverage          ?   75.25%           
=========================================
  Files             ?      309           
  Lines             ?    14341           
  Branches          ?     2540           
=========================================
  Hits              ?    10793           
  Misses            ?     3538           
  Partials          ?       10

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 6af8f99...3af012e. Read the comment docs.

DonJayamanne · 2018-08-01T19:03:04Z

Close and reopen for VSTS

ericsnowcurrently · 2018-08-01T20:15:54Z

+    # 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 \
+        source.endswith('\n\n') or source.endswith('\r\n\r\n'):


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. :)

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.

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

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

I.e. cross platform support

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

ericsnowcurrently · 2018-08-01T20:16:35Z

+    # 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 \


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

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.

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')?

ericsnowcurrently · 2018-08-01T20:20:29Z

+    # 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 \
+        source.endswith('\n\n') or source.endswith('\r\n\r\n'):


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

ericsnowcurrently · 2018-08-01T20:23:52Z

+    # 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 \


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.

ericsnowcurrently · 2018-08-01T20:24:57Z

+    # we have indented code.
+    if (len(lines) > 1 and len(''.join(lines[-2:])) == 0) or \
+        source.endswith('\n\n') or source.endswith('\r\n\r\n'):
+        trailing_newline = os.linesep * 2


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

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

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?

ericsnowcurrently · 2018-08-01T20:27:14Z

    # 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'):


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

Please explain why.

ericsnowcurrently · 2018-08-01T20:28:21Z

+    # 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 \
+        source.endswith('\n\n') or source.endswith('\r\n\r\n'):


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

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

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

ericsnowcurrently · 2018-08-01T20:28:48Z

    # 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'):


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

DonJayamanne · 2018-08-02T23:10:40Z

@ericsnowcurrently
Please re-review

DonJayamanne · 2018-08-11T16:11:45Z

@Microsoft/pvsc-team please could someone re-review this PR

ericsnowcurrently · 2018-08-14T15:41:49Z

+        trailing_newline = os.linesep
    else:
        trailing_newline = ''



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.

ericsnowcurrently · 2018-08-14T15:42:49Z

+    # 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) \


I still think (len(lines) > 1 and len(''.join(lines[-2:])) == 0) is unnecessary.

To account for spaces

ericsnowcurrently · 2018-08-14T15:43:30Z

    # 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', '\r\n')):


Likewise, len(lines[-1].strip()) == 0 is unnecessary, no?

To account for spaces

ericsnowcurrently · 2018-08-14T15:45:19Z

+    # we have indented code.
+    if (len(lines) > 1 and len(''.join(lines[-2:])) == 0) or \
+        source.endswith('\n\n') or source.endswith('\r\n\r\n'):
+        trailing_newline = os.linesep * 2


ericsnowcurrently

Other than the question of why the first condition is necessary, LGTM. Please address that before merging this.

brettcannon · 2018-08-20T17:14:47Z

@DonJayamanne can we merge this?

aminevsaziz · 2018-10-02T23:28:29Z

@DonJayamanne This issue still exists for me.

d3r3kk · 2018-10-03T00:14:41Z

@aminevsaziz Please open a new issue on our github if this issue isn't resolved for you, we can always use the help tracking down and fixing bugs! (Note, I've edited your comment to come off less 👿 and more 😺).

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

aa6c342

ericsnowcurrently self-assigned this Aug 1, 2018

DonJayamanne closed this Aug 1, 2018

DonJayamanne reopened this Aug 1, 2018

Tweak news entry

b00acb0

brettcannon requested a review from ericsnowcurrently August 1, 2018 20:01

ericsnowcurrently suggested changes Aug 1, 2018

View reviewed changes

ericsnowcurrently removed their assignment Aug 1, 2018

ericsnowcurrently mentioned this pull request Aug 2, 2018

Shift+Enter doesn't fully run code when last line of code is indented #2167

Closed

Address code review issues

c4a7d02

ericsnowcurrently reviewed Aug 14, 2018

View reviewed changes

ericsnowcurrently approved these changes Aug 14, 2018

View reviewed changes

Fix code review comments

3af012e

DonJayamanne merged commit b5e1e44 into microsoft:master Sep 6, 2018

DonJayamanne deleted the issue2167NewLine branch October 2, 2018 22:46

lock Bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Conversation

DonJayamanne commented Jul 21, 2018

Uh oh!

codecov Bot commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DonJayamanne commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne commented Aug 2, 2018

Uh oh!

DonJayamanne commented Aug 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Aug 20, 2018

Uh oh!

aminevsaziz commented Oct 2, 2018 • edited by d3r3kk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d3r3kk commented Oct 3, 2018

codecov Bot commented Jul 21, 2018 •

edited

Loading

ericsnowcurrently Aug 1, 2018 •

edited

Loading

aminevsaziz commented Oct 2, 2018 •

edited by d3r3kk

Loading