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.

Sign up for GitHub

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

Jump to bottom

Semantic Snippets - don't show in razor documents #64377

Closed
wants to merge 1 commit into from
Closed

Semantic Snippets - don't show in razor documents #64377

wants to merge 1 commit into from

Conversation

akhera99
Copy link
Member

Reverts back to the original behavior
image

@akhera99 akhera99 requested a review from a team as a code owner September 29, 2022 22:14
jasonmalinowski
jasonmalinowski reviewed Sep 29, 2022
View reviewed changes
src/Features/CSharp/Portable/Completion/CompletionProviders/SnippetCompletionProvider.cs
@@ -157,7 +157,7 @@ SyntaxKind.EndRegionKeyword or
return ImmutableArray<CompletionItem>.Empty;

var snippets = service.GetSnippetsIfAvailable();
if (context.CompletionOptions.ShouldShowNewSnippetExperience())
if (context.CompletionOptions.ShouldShowNewSnippetExperience() && !context.Document.IsRazorDocument())
Copy link
Member

Choose a reason for hiding this comment

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

Should this be rolled into ShouldShowNewSnippetExperience?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe we have access to the document at that point?

Copy link
Member

Choose a reason for hiding this comment

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

Could you pass in the Document as an argument to ShouldShowNewSnippetExperience?

@davidwengier
Copy link
Contributor

Needs to merge into release/17.4?

@davidwengier davidwengier mentioned this pull request Sep 29, 2022
Open
@akhera99 akhera99 changed the base branch from main to release/dev17.4 September 29, 2022 22:20
@akhera99 akhera99 requested review from a team as code owners September 29, 2022 22:20
@akhera99 akhera99 removed request for a team September 29, 2022 22:24
@akhera99 akhera99 changed the base branch from release/dev17.4 to main September 29, 2022 22:25
@DustinCampbell
Copy link
Member

Needs to merge into release/17.4?

Yup, yup!

@akhera99 akhera99 mentioned this pull request Sep 29, 2022
Merged
ToddGrun
ToddGrun reviewed Sep 29, 2022
View reviewed changes
src/Features/CSharp/Portable/Completion/CompletionProviders/SnippetCompletionProvider.cs
@@ -157,7 +157,7 @@ SyntaxKind.EndRegionKeyword or
return ImmutableArray<CompletionItem>.Empty;

var snippets = service.GetSnippetsIfAvailable();
if (context.CompletionOptions.ShouldShowNewSnippetExperience())
if (context.CompletionOptions.ShouldShowNewSnippetExperience() && !context.Document.IsRazorDocument())
Copy link
Contributor

Choose a reason for hiding this comment

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

IsRazorDocument

Does this work for legacy razor files too?

@CyrusNajmabadi
Copy link
Member

Reverts back to the original behavior

Could you add a comment explaining why this is desirable? Why don't we want semantic snippets in razor docs?

@akhera99
Copy link
Member Author

Reverts back to the original behavior

Could you add a comment explaining why this is desirable? Why don't we want semantic snippets in razor docs?

dotnet/razor#6927
We do, but it will be done in 17.5

@akhera99
Copy link
Member Author

closed with #64378

@akhera99 akhera99 closed this Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

@genlu genlu genlu left review comments

@dibarbet dibarbet dibarbet left review comments

@ToddGrun ToddGrun ToddGrun left review comments

@davidwengier davidwengier Awaiting requested review from davidwengier

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@akhera99 @davidwengier @DustinCampbell @CyrusNajmabadi @jasonmalinowski @genlu @dibarbet @ToddGrun