-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Ensure code is executed when last line of selected code is indented #2222
Conversation
Codecov Report
@@ 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.
|
Close and reopen for VSTS |
# 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'): |
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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! :)
# 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 \ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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')
?
# 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'): |
There was a problem hiding this comment.
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.
# 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 \ |
There was a problem hiding this comment.
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.
# 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain why.
# 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'): |
There was a problem hiding this comment.
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'))
There was a problem hiding this comment.
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...
# 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'): |
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@ericsnowcurrently |
@Microsoft/pvsc-team please could someone re-review this PR |
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')): | ||
trailing_newline = os.linesep | ||
else: | ||
trailing_newline = '' | ||
|
There was a problem hiding this comment.
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.
# 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) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think (len(lines) > 1 and len(''.join(lines[-2:])) == 0)
is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To account for spaces
# 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')): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, len(lines[-1].strip()) == 0
is unnecessary, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To account for spaces
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the question of why the first condition is necessary, LGTM. Please address that before merging this.
@DonJayamanne can we merge this? |
@DonJayamanne This issue still exists for me. |
@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 😺). |
Fixes #2167
"1.2.3"
, not"^1.2.3"
)package-lock.json
has been regenerated if dependencies have changed