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

Support auto-closing quotes in Python raw string literals, etc #35636

Merged
merged 1 commit into from Nov 30, 2017

Conversation

Projects
None yet
7 participants
@Syeberman

Syeberman commented Oct 5, 2017

Python uses a "string prefix" to denote bytes literals (b''), raw strings (r''), and formatted string literals (f''). This change makes it so that when one types r' in VS Code, the string will be auto-closed as r'' (as an example).

Python also supports multiple string prefixes for a single literal. I've tested that typing fr' will auto-close to fr'', and so forth.

Sye van der Veen
Support auto-closing quotes in Python raw string literals, etc
Python uses a ["string prefix"](https://docs.python.org/3/reference/lexical_analysis.html#string-and-bytes-literals) to denote bytes literals (`b''`), raw strings (`r''`), and formatted string literals (`f''`).  This change makes it so that when one types `r'` in VS Code, the string will be auto-closed as `r''` (as an example).

Python also supports multiple string prefixes for a single literal.  I've tested that typing `fr'` will auto-close to `fr''`, and so forth.
@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Oct 5, 2017

CLA assistant check
All CLA requirements met.

msftclas commented Oct 5, 2017

CLA assistant check
All CLA requirements met.

{ "open": "f'", "close": "'", "notIn": ["string", "comment"] },
{ "open": "F'", "close": "'", "notIn": ["string", "comment"] },
{ "open": "b'", "close": "'", "notIn": ["string", "comment"] },
{ "open": "B'", "close": "'", "notIn": ["string", "comment"] }

This comment has been minimized.

@javadev

javadev Oct 6, 2017

It may be unit tested.

@javadev

javadev Oct 6, 2017

It may be unit tested.

@aeschli

This comment has been minimized.

Show comment
Hide comment
@aeschli

aeschli Oct 6, 2017

Contributor

Isn't it enough to just have ' and " as an auto pair character? The only difference I'm aware of is that undo will only remove the quote, instead of the letter before, but IMO that's better anyway.

Contributor

aeschli commented Oct 6, 2017

Isn't it enough to just have ' and " as an auto pair character? The only difference I'm aware of is that undo will only remove the quote, instead of the letter before, but IMO that's better anyway.

@Syeberman

This comment has been minimized.

Show comment
Hide comment
@Syeberman

Syeberman Oct 6, 2017

The original auto-close setting did not auto-close r', etc. Presumably, it's coded to only trigger after a whitespace (or non-word boundary), but I didn't look at the code.

Oddly, this doesn't explain why the above configuration auto-closes fr', etc. If the open pattern must be preceded by whitespace, then that shouldn't work. Perhaps it's coded differently if the pattern starts with an alphanumeric character?

Syeberman commented Oct 6, 2017

The original auto-close setting did not auto-close r', etc. Presumably, it's coded to only trigger after a whitespace (or non-word boundary), but I didn't look at the code.

Oddly, this doesn't explain why the above configuration auto-closes fr', etc. If the open pattern must be preceded by whitespace, then that shouldn't work. Perhaps it's coded differently if the pattern starts with an alphanumeric character?

@aeschli

This comment has been minimized.

Show comment
Hide comment
@aeschli

aeschli Oct 12, 2017

Contributor

Yes, there's a condition to not autoclose ' after a word.
@alexandrudima Do you want to reconsider that, or is the PR ok?

Contributor

aeschli commented Oct 12, 2017

Yes, there's a condition to not autoclose ' after a word.
@alexandrudima Do you want to reconsider that, or is the PR ok?

@aeschli aeschli added this to the October 2017 milestone Oct 12, 2017

@aeschli aeschli modified the milestones: October 2017, November 2017 Nov 3, 2017

@aeschli

This comment has been minimized.

Show comment
Hide comment
@aeschli

aeschli Nov 3, 2017

Contributor

Sorry, this slipped. @alexandrudima Waiting for your opinion.

Contributor

aeschli commented Nov 3, 2017

Sorry, this slipped. @alexandrudima Waiting for your opinion.

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 14, 2017

Member

The following added rules do nothing:

		{ "open": "r\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "R\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "u\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "U\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "f\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "F\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "b\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "B\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "r'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "R'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "u'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "U'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "f'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "F'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "b'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "B'", "close": "'", "notIn": ["string", "comment"] }

Out of the added rules, this is the only one that might do something:

		{ "open": "'", "close": "'", "notIn": ["string", "comment"] },

Closing Pairs are implemented to be working on single characters. Basically, the code evaluates if someone typed the character in the open part of a closing pair and does not consider any characters before the auto closing open character.

Member

alexandrudima commented Nov 14, 2017

The following added rules do nothing:

		{ "open": "r\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "R\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "u\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "U\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "f\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "F\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "b\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "B\"", "close": "\"", "notIn": ["string", "comment"] },
		{ "open": "r'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "R'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "u'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "U'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "f'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "F'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "b'", "close": "'", "notIn": ["string", "comment"] },
		{ "open": "B'", "close": "'", "notIn": ["string", "comment"] }

Out of the added rules, this is the only one that might do something:

		{ "open": "'", "close": "'", "notIn": ["string", "comment"] },

Closing Pairs are implemented to be working on single characters. Basically, the code evaluates if someone typed the character in the open part of a closing pair and does not consider any characters before the auto closing open character.

@Syeberman

This comment has been minimized.

Show comment
Hide comment
@Syeberman

Syeberman Nov 14, 2017

@alexandrudima I tested this configuration myself when I submitted this PR. Without my changes, when you type r', it does not auto-close to r''. With my changes, it properly auto-closes raw, unicode, f-strings, and bytes.

Syeberman commented Nov 14, 2017

@alexandrudima I tested this configuration myself when I submitted this PR. Without my changes, when you type r', it does not auto-close to r''. With my changes, it properly auto-closes raw, unicode, f-strings, and bytes.

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 14, 2017

Member

Yes, that's what I'm saying too. The only line that makes this work from this PR is:

{ "open": "'", "close": "'", "notIn": ["string", "comment"] },

This defines that ' - ' forms an auto-closing pair.

The other 16 lines serve no purpose.

Member

alexandrudima commented Nov 14, 2017

Yes, that's what I'm saying too. The only line that makes this work from this PR is:

{ "open": "'", "close": "'", "notIn": ["string", "comment"] },

This defines that ' - ' forms an auto-closing pair.

The other 16 lines serve no purpose.

@aeschli

This comment has been minimized.

Show comment
Hide comment
@aeschli

aeschli Nov 15, 2017

Contributor

@alexandrudima
{ "open": "'", "close": "'", "notIn": ["string", "comment"] }, is already part of the autoClosingPair.
It doesn't work after r' due to this.
Adding { "open": "r'", "close": "'", "notIn": ["string", "comment"] }, fixes the problem

Contributor

aeschli commented Nov 15, 2017

@alexandrudima
{ "open": "'", "close": "'", "notIn": ["string", "comment"] }, is already part of the autoClosingPair.
It doesn't work after r' due to this.
Adding { "open": "r'", "close": "'", "notIn": ["string", "comment"] }, fixes the problem

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 16, 2017

Member

@Syeberman @aeschli I agree the change makes this feature work. I was quite stumped at first, since I am pretty sure how auto-closing characters should work and how they're implemented.

As I thought, the execution does not enter the auto closing open character code (since that is implemented to only work on single characters), but it enters BracketElectricCharacterSupport._onElectricAutoClose and these newly added auto-closing pairs somehow make their way to _complexAutoClosePairs.

@aeschli Since this is something you've authored in ebc3074 , I cannot be of further assistance.

Perhaps now would be a good time to document somewhere this behaviour!

Member

alexandrudima commented Nov 16, 2017

@Syeberman @aeschli I agree the change makes this feature work. I was quite stumped at first, since I am pretty sure how auto-closing characters should work and how they're implemented.

As I thought, the execution does not enter the auto closing open character code (since that is implemented to only work on single characters), but it enters BracketElectricCharacterSupport._onElectricAutoClose and these newly added auto-closing pairs somehow make their way to _complexAutoClosePairs.

@aeschli Since this is something you've authored in ebc3074 , I cannot be of further assistance.

Perhaps now would be a good time to document somewhere this behaviour!

@aeschli aeschli merged commit d4d3d40 into Microsoft:master Nov 30, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@aeschli

This comment has been minimized.

Show comment
Hide comment
@aeschli

aeschli Nov 30, 2017

Contributor

Talked to @alexandrudima again and we are fine with the PR. Not autoclosing ' and " after a letter is the expected behaviour most languages, so the PR makes sense to add additional rules fo the cases where this makes sense.

Thanks @Syeberman!

Contributor

aeschli commented Nov 30, 2017

Talked to @alexandrudima again and we are fine with the PR. Not autoclosing ' and " after a letter is the expected behaviour most languages, so the PR makes sense to add additional rules fo the cases where this makes sense.

Thanks @Syeberman!

@diazgilberto

This comment has been minimized.

Show comment
Hide comment
@diazgilberto

diazgilberto Mar 23, 2018

I still have the issue on Version 1.21.1 (1.21.1) and running OSX Sierra.

diazgilberto commented Mar 23, 2018

I still have the issue on Version 1.21.1 (1.21.1) and running OSX Sierra.

@Syeberman Syeberman deleted the Syeberman:python_auto_closing_string_prefix branch Mar 23, 2018

@aeschli

This comment has been minimized.

Show comment
Hide comment
@aeschli

aeschli Mar 26, 2018

Contributor

@diazgilberto Please file a new issue with a code sample that shows the problem. Also try to run without extensions to see whether an external extension causes this.

Contributor

aeschli commented Mar 26, 2018

@diazgilberto Please file a new issue with a code sample that shows the problem. Also try to run without extensions to see whether an external extension causes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment