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

Remove Razor and editor inference document option providers #61091

Merged
merged 5 commits into from
May 13, 2022
Merged

Remove Razor and editor inference document option providers #61091

merged 5 commits into from
May 13, 2022

Conversation

tmat
Copy link
Member

@tmat tmat commented May 2, 2022 •
edited
Loading

Follow up on #61054 and dotnet/razor#6338.

The idea is that instead of Razor providing formatting options for design-time documents using IDocumentOptionsProvider implement a Razor specific StructuredAnalyzerOptionsProvider for the documents. This is a workaround until Razor switches all formatting logic to LSP.

Similarly replaces InferredIndentationDocumentOptionsProviderFactory with a custom analyzer options provider. Also a workaround that should go away - tracked by #61109.

The goal is to ultimately remove IDocumentOptionsProvider extensibility point.

@tmat tmat requested review from a team as code owners May 2, 2022 20:44
@tmat
Copy link
Member Author

tmat commented May 2, 2022

@jasonmalinowski Does this look ok?

@tmat tmat changed the title Razor line formatting options remove document service Remove Razor line document options provider service May 2, 2022
jasonmalinowski
jasonmalinowski reviewed May 2, 2022
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Besides some specific concerns, I admit I'm not seeing the big picture here. Sure, this gets rid of some document services, but seems like this just couples in a different way and I'm not sure what the goal is for that long term.

src/EditorFeatures/Core/Options/LegacyGlobalOptionsWorkspaceService.cs Show resolved Hide resolved
src/Workspaces/Core/Portable/Workspace/Host/DocumentService/Extensions.cs Outdated Show resolved Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs Show resolved Hide resolved
@jasonmalinowski
Copy link
Member

@tmat: the compiler sure doesn't. 😉

@tmat
Copy link
Member Author

tmat commented May 2, 2022 •
edited
Loading

Besides some specific concerns, I admit I'm not seeing the big picture here. Sure, this gets rid of some document services, but seems like this just couples in a different way and I'm not sure what the goal is for that long term.

This is still a workaround, for sure. It works better in the new options design though, where features do not use document.GetOptionsAsync to read options. Instead they read editorconfig options directly and fall back to defaults stored in global options.

Long term we get the right options through LSP.

@tmat tmat force-pushed the RazorLineFormattingOptions-RemoveDocumentService branch from 6a047ed to de4064f Compare May 2, 2022 23:16
@tmat tmat marked this pull request as draft May 2, 2022 23:16
@tmat tmat changed the title Remove Razor line document options provider service Remove Razor and editor inference document option providers May 3, 2022
@tmat tmat force-pushed the RazorLineFormattingOptions-RemoveDocumentService branch 3 times, most recently from 5202c1d to f2f0b8e Compare May 10, 2022 02:07
@tmat tmat marked this pull request as ready for review May 10, 2022 18:56
@tmat tmat force-pushed the RazorLineFormattingOptions-RemoveDocumentService branch from b66b174 to 4e0d019 Compare May 12, 2022 16:12
dibarbet
dibarbet approved these changes May 12, 2022
View reviewed changes
src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs
}

/// <summary>
/// Provides editorconfig options for Razor design-time documents.
Copy link
Member

Choose a reason for hiding this comment

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

when do we need options on the razor design time documents? If they're asking us for formatting in razor, they should be p[assing the tabs/spaces/size options to us. If there are cases where we're not passing the options / using the passed in topions, is there a bug tracking it?
cc @NTaylorMullen

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I'm not entirely sure. I think there might cases when a Roslyn feature might trigger change in a Razor document.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidwengier

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I forget. @allisonchou do you recall?

...es/SharedUtilitiesAndExtensions/Compiler/Core/Diagnostics/StructuredAnalyzerConfigOptions.cs Outdated
// StructuredAnalyzerConfigOptions is defined in both Worksapce and Code Style layers. It is not public and thus can't be shared between these two.
// However, Code Style layer is compiled against the shared Workspace APIs. The ProjectState creates and holds onto an instance
// of Workspace layer's version of StructuredAnalyzerConfigOptions. This version of the type is not directly usable by Code Style code.
// We create a clone of this instance typed to the Code Style's version of StructuredAnalyzerConfigOptions.
Copy link
Member

Choose a reason for hiding this comment

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

:( this codestyle stuff turns my brain into goo

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. It's very complex. Hopefully after next few PRs it's gonna get a bit simpler.

@tmat tmat force-pushed the RazorLineFormattingOptions-RemoveDocumentService branch from 4e0d019 to a6d0e49 Compare May 13, 2022 00:40
@tmat tmat enabled auto-merge (squash) May 13, 2022 01:29
@tmat tmat merged commit 9839a67 into dotnet:main May 13, 2022
@ghost ghost added this to the Next milestone May 13, 2022
333fred added a commit to 333fred/roslyn that referenced this pull request May 16, 2022
@333fred
Merge remote-tracking branch 'upstream/features/required-members' int…
49b94a8 
…o poison

* upstream/features/required-members: (413 commits)
  [EE] Implement IDkmClrFullNameProvider2 in Roslyn's ResultProvider Formatter. (dotnet#60522)
  Remove parameter null-checking from the Language Feature Status list (dotnet#61302)
  Add pointer for `AnalysisLevel` to warning waves doc (dotnet#61196)
  Add an UWP OptProf test for IDE
  Add test
  Fix issue where we were getting a raw-string in a skipped token, causing a crash
  Fix several LSP completion kind mappings (dotnet#61243)
  Relax assertion in SyntheticBoundNodeFactory.Convert (dotnet#61287)
  Enable add usings on paste by default (dotnet#61299)
  Fix focus on rename UI opening (dotnet#60846)
  Update PublishData.json with new package
  Remove Razor and editor inference document option providers (dotnet#61091)
  Fix nested in generic type binding issues in enabled nullability context (dotnet#61182)
  Revert "Revert "Improve editorconfig options caching on ProjectState (dotnet#61131)" (dotnet#61216)" (dotnet#61283)
  Move MSBuild back to 16.5.0.
  Fix a few build and package issues (dotnet#61273)
  lint
  Revert workspaces msbuild changes.
  Use SegmentedHashSet<T> to eliminate LOH allocations in AsyncBatchingWorkQueue
  NRT
  ...
@Cosifne Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022
@arunchndr arunchndr mentioned this pull request Nov 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NTaylorMullen NTaylorMullen NTaylorMullen left review comments

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

@dibarbet dibarbet dibarbet approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@tmat @jasonmalinowski @NTaylorMullen @dibarbet @Cosifne