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.

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

Fix commit behvaior where items use default commit chars as their special filter char #70743

Merged
merged 1 commit into from
Nov 11, 2023

Conversation

genlu
Copy link
Member

@genlu genlu commented Nov 9, 2023

We never handled this correctly, but the bug wasn't exposed until operator completion was added (where '+', '-', etc. are used as filter chars)

Fix #70732

…cial filter char

We never handled this correctly but the bug wasn't exposed until operator comptetion was added (where '+', '-', etc. are used as filter chars)
@genlu genlu requested a review from a team as a code owner November 9, 2023 19:57
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 9, 2023
{
if (session.Properties.TryGetProperty(ExcludedCommitCharacters, out ImmutableArray<char> excludedCommitCharactersBefore))
foreach (var kvp in excludedCommitCharactersFromList)
Copy link
Contributor

@ToddGrun ToddGrun Nov 10, 2023

Choose a reason for hiding this comment

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

foreach (var kvp in excludedCommitCharactersFromList)

nit: would be nice if there were an AddRange on MultiDictionary. Would simplify this code and it's implementation would be slightly more performant as it would only need to look up each key once

Copy link
Member Author

@genlu genlu Nov 10, 2023

Choose a reason for hiding this comment

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

I think AddRange is usually associated with list like types in BCL. Maybe we could add a Union for MultiDictionary? I can give this a try in a follow up PR since it'd require change in compiler layer. Also import compeltion uses this type extensively so I'd like to take that into consideration too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, I just saw that OrderedMultiDictionary has an AddRange, so I thought the name could match that, but Union is fine by me too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, this method must get called on the same session multiple times with different completionList.ItemsList values, right? Just trying to understand if there is a relation between previous values in the ExcludedCommitCharactersMap property and the items returned from GetExcludedCommitCharacters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, currently it could be called twice for a given session to support import completion, as we might delay those unimported item to a second batch w/o showing them until the list refreshed, or if import completion is disabled, it can be explicitly triggered by using the expander button during the same session. Which means the values in the map from before vs after are from two disjoint sets of completion items (unimported item vs everything else).

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

@genlu genlu merged commit e6318f0 into dotnet:main Nov 11, 2023
24 checks passed
@genlu genlu deleted the FixCommit branch November 11, 2023 00:04
@ghost ghost added this to the Next milestone Nov 11, 2023
@RikkiGibson RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C# editor auto complete fails, if followed by an operator
3 participants