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

Options Refactoring - Round 3 #57254

Merged
merged 17 commits into from
Nov 1, 2021
Merged

Options Refactoring - Round 3 #57254

merged 17 commits into from
Nov 1, 2021

Conversation

tmat
Copy link
Member

@tmat tmat commented Oct 20, 2021

More options refactoring applying patterns described in #55728

Introduces attributes ExportSolutionOptionProvider and ExportGlobalOptionProvider that derive from ExportOptionProvider but does not use them yet when importing options. Will follow up on that in a separate PR (issue #42308).

Follow-up: #57283

@tmat tmat changed the title Options5 Options Refactoring - Round 3 Nov 1, 2021
@tmat tmat marked this pull request as ready for review November 1, 2021 20:38
@tmat tmat requested review from a team as code owners November 1, 2021 20:38
@@ -62,12 +62,6 @@ public int NavigateToDecompiledSources
set { SetBooleanOption(FeatureOnOffOptions.NavigateToDecompiledSources, value); }
}

public int UseEnhancedColors
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for getting this!

public static readonly PerLanguageOption2<bool> EnableInlineDiagnostics =
new(nameof(InlineDiagnosticsOptions),
nameof(EnableInlineDiagnostics),
new("InlineDiagnosticsOptions",
Copy link
Member

Choose a reason for hiding this comment

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

Why is not good to use nameof() here? Easier to move options between code files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because renaming the internal type might break the option (change the key that's used to look it up). There is really no relationship that we need to maintain between the type that declares the option and the option key.

@tmat tmat merged commit b0f460c into dotnet:main Nov 1, 2021
@ghost ghost added this to the Next milestone Nov 1, 2021
@tmat tmat deleted the Options5 branch November 1, 2021 21:24
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 2, 2021
* upstream/main: (51 commits)
  Disable sql when built from source
  Fix
  Reduce indirections
  Reduce indirections
  Rename files
  Fix nullability warning
  Update cloud cache in the same way
  Ensure only one storage service factory gets created per workspace.
  Remove processed typename check
  Simplify Equals method
  Build CodeAction description into table and only generate Dev17 hash
  Avoid recomputing expensive persistence location data that does not change.
  Options Refactoring - Round 3 (dotnet#57254)
  Merge pull request dotnet#57509 from dotnet/dev/rigibson/fix-publishdata
  Classify method group assignments as methods (dotnet#57410)
  Simplify
  NRT
  Simplify
  Remove unnecessary workaround code.
  Simplify
  ...
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
@ryzngard ryzngard added UX Review Not Required UX Review Not Required and removed Needs UX Triage labels Apr 21, 2022
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.

6 participants