Skip to content

Refactor indentation API#57529

Open
tmat wants to merge 2 commits into
dotnet:mainfrom
tmat:AsyncIndentation
Open

Refactor indentation API#57529
tmat wants to merge 2 commits into
dotnet:mainfrom
tmat:AsyncIndentation

Conversation

@tmat
Copy link
Copy Markdown
Member

@tmat tmat commented Nov 2, 2021

Hoist WaitAndGetResult to the invoking command handler on one code path and avoid it completely in others.

Depends on #58904

@ghost ghost added the Area-IDE label Nov 2, 2021
@tmat tmat marked this pull request as ready for review November 2, 2021 21:02
@tmat tmat requested review from a team as code owners November 2, 2021 21:02
@sharwell
Copy link
Copy Markdown
Contributor

sharwell commented Nov 2, 2021

Seems we've been undecisive here

b80b704
8836b5b

Comment thread src/Features/CSharp/Portable/SplitStringLiteral/StringSplitter.cs Outdated
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

I don't have an answer here on what we should be doing. However, this needs to be discussed as there is high chance this can lead to problems (which we dealt with in the past, leading to the current design). Tagging @jasonmalinowski @sharwell

Comment thread src/EditorFeatures/CSharp/SplitStringLiteral/SplitStringLiteralCommandHandler.cs Outdated
@jasonmalinowski
Copy link
Copy Markdown
Member

Yeah, we are keeping this path sync for the typing performance reason as @CyrusNajmabadi correctly calls out -- what would this look like to have both sync/async paths? Or do we have some better way to structure the code?

@tmat tmat changed the title Make indentation API async Refactor indentation API Nov 3, 2021
@tmat
Copy link
Copy Markdown
Member Author

tmat commented Nov 3, 2021

@CyrusNajmabadi @jasonmalinowski How does it look now?

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Nov 3, 2021

See #57553 for future direction

@tmat tmat force-pushed the AsyncIndentation branch from 916555c to 1ce822a Compare January 21, 2022 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants