Conversation
0631b99 to
139bc3e
Compare
|
@ryzngard @jasonmalinowski @CyrusNajmabadi @dotnet/roslyn-ide PTAL |
They don't have to be added to Imho it seems to be a generally appropriate location for feature flags. |
I see. That makes sense. I think we should remove our options from FeatureFlags.xml and put them in PackageRegistration.pkgdef then. It'd be good to have a single place for them. |
src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/Options/FeatureFlagPersister.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/Options/FeatureFlagPersisterProvider.cs
Show resolved
Hide resolved
src/VisualStudio/IntegrationTest/TestUtilities/VisualStudioInstanceFactory.cs
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/FeatureFlagStorageLocation.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Approved given confirmation about the threadign questions, and a strong ask that you make the change here: https://github.com/dotnet/roslyn/pull/55902/files#r698729640
c3e743b to
ea6a640
Compare
3a176a6 to
3125c4c
Compare
Implements option persisters for feature flags and replaces calls to
IExperimentationServicewith reads/writes of the corresponding options.As with other options, feature flag options defined in Workspaces and Features layers are serialized and available OOP via
Solution.Options. All other feature flag options are only available in-proc. This means the feature flag options need to be defined along the feature implementation, not in central location likeWellKnownExperimentNames.The previous implementation of IExperimentationService in VS layer conflated feature flags and experimental flights.
My understanding is following:
IVsExperimentationService.IsCachedFlightEnabledchecks whether the user is selected to participate in a specified flight name. It has nothing to do with feature flags.IVsFeatureFlags.IsFeatureEnabledchecks a feature flag that's stored in VS feature flag storage.VS provides two services:
I believe we should not check flight names directly in order to enable/disable a feature but always use a feature flag. This PR deletes calls to
IVsExperimentationService.IsCachedFlightEnabled, which checks for active flight name.Active feature flags:
Only two feature flags are listed in
src/vscommon/featureflags/FeatureFlags.xml4 are listed in PackageRegistration.pkgdef These are displayed in Options > Preview Features. Should we present all feature flags there instead of having some of them displayed in C#/VB specific option panes? Follow up: #55974Unused feature flags (deleted by this PR):
Flight names (Control Tower reports as completed and no longer running, deleted by this PR) - not clear if we still need
KeybindingResetDetectorat all. Following up.Fixes #43477