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
Remove the feature flag and change the default for open generated files #60976
base: main
Are you sure you want to change the base?
Remove the feature flag and change the default for open generated files #60976
Conversation
Right now creating this to ensure no functional/performance regressions in divisional tests before we flip the feature flag. This will merge at some later point when we're comfortable with it. |
This comment was marked as outdated.
This comment was marked as outdated.
5fff088
to
f9f154e
Compare
Internal validation run passed, so this is ready for review. |
This def worries me. But i guess we have feature flag to roll back if this causes an issue? |
new FeatureFlagStorageLocation("Roslyn.SourceGeneratorsEnableOpeningInWorkspace")); | ||
public static readonly Option2<bool> EnableOpeningSourceGeneratedFilesInWorkspace = new( | ||
"WorkspaceConfigurationOptions", "EnableOpeningSourceGeneratedFilesInWorkspace", defaultValue: true, | ||
new RoamingProfileStorageLocation("TextEditor.Roslyn.Specific.EnableOpeningSourceGeneratedFilesInWorkspace")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't see a feature-flag anymore. that def worries me if we're late in the game here. turning this on by default (when SGs have been a huge source of perf issues across the board for the IDE seems concerning to me).
Ok. So i'm fine flipping this on by default. But i'd say we should still have an available flag to disable thsi remotely if it turns out to be an issue. |
@CyrusNajmabadi Actually in that case then I can just flip the existing feature flag on, and we can just merge this in 17.6. |
That works for me. |
This is being merged into 17.6, but the existing feature flag is being flipped for 17.5. |
Target main since it is 17.6 now |
@CyrusNajmabadi So we've had this feature flag rolled out to 100% of users for awhile with few reported issues so I'd like to merge this for 17.7; I don't see any reason to keep this feature flag around at this point. |
Works for me! |
This is no longer experimental.
For some reason we have a method that tests use to clear the cached value if requisite options change. Rather than doing that we can just clear it automatically; without this the option is also frozen for the lifetime of the process.
f9f154e
to
c259bc9
Compare
@CyrusNajmabadi Ready for review then after resolving merge conflicts. |
No description provided.