Skip to content

Conversation

@gabritto
Copy link
Member

@gabritto gabritto commented Aug 2, 2024

Fixes #31244.
Fixes #25691.

As I said in #59339, our existing behavior, implemented based on vscode's behavior, is that whenever isNewIdentifierLocation is true, we disable all commit characters, otherwise the commit characters are ., ,, ;.

However, there have been issues opened over time that this approach is too conservative.
This PR enables some of the usual commit characters in some places where isNewIdentifierLocation is true and that previously we'd enable no commit characters.

Mainly, this PR enables all the three commit characters in every position where an expression is expected. In some of those positions, where we are inside an open parenthesis, the user could be typing a parameter list for an arrow expression, so we disable , as a commit character.

I considered adding space as a commit character too, but we would have to disable it pretty much everywhere where isNewIdentifierLocation is true because space could validly follow a new identifier in all of those places, so I figured users wouldn't be able to use it reliably as a commit character and it didn't seem like a good idea.

This PR should be testable in vscode insiders once it picks up microsoft/vscode#223541.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Aug 2, 2024
@gabritto gabritto changed the title Enable existing commit characters in certain locations where `is Enable existing commit characters in certain locations where isNewIdentifier is true Aug 2, 2024
@gabritto gabritto marked this pull request as ready for review August 3, 2024 00:02
@gabritto
Copy link
Member Author

gabritto commented Aug 3, 2024

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 3, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 3, 2024

Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/163138/artifacts?artifactName=tgz&fileId=4C1EFBDE2A0665C56E75E44A26218F8291D850382E267E20EB350493D7B12E0202&fileName=/typescript-5.6.0-insiders.20240803.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.6.0-pr-59523-2".;

@gabritto
Copy link
Member Author

gabritto commented Aug 6, 2024

This should be testable using vscode insiders and setting TS to this PR (see this).

@gabritto gabritto merged commit 1e7889c into main Aug 19, 2024
@gabritto gabritto deleted the gabritto/issue31244 branch August 19, 2024 20:24
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suggestions not accepted when using commit characters isNewIdentifierLocation is always set to true inside function call

4 participants