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

Hide replace/original functionality behind a feature flag. #11157

Merged
merged 1 commit into from
May 9, 2016

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented May 7, 2016

Since "original" is by itself contextual on the presence of "replace", only "replace" actually needs to be keyed off a flag.

Since "original" is by itself contextual on the presence of "replace", only "replace" actually needs to be keyed off a flag.
@VSadov
Copy link
Member Author

VSadov commented May 7, 2016

@cston @jcouv @dotnet/roslyn-compiler - please review.

@VSadov
Copy link
Member Author

VSadov commented May 9, 2016

note that, similarly to analyzers, invoking of actual generators is not conditional on language version/features.

@@ -14,6 +14,16 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Symbols
{
public class ReplaceOriginalTests : CSharpTestBase
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider marking tests with the feature attribute.

Copy link
Member

Choose a reason for hiding this comment

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

@AlekseyTs agree. This is now a pre-req before merging to future so let's do this now.

@AlekseyTs
Copy link
Contributor

LGTM

@jaredpar
Copy link
Member

jaredpar commented May 9, 2016

LGTM modulo attribute comment.

private static CSharpCompilation CreateCompilationWithMscorlib(string text, CSharpCompilationOptions options)
{
return CreateCompilationWithMscorlib(text, options: options, parseOptions: TestOptions.Regular.WithReplaceFeature());
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider using options = null and removing the other overload.

Copy link
Member Author

Choose a reason for hiding this comment

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

done in a new change. Thanks

@cston
Copy link
Member

cston commented May 9, 2016

LGTM

@VSadov
Copy link
Member Author

VSadov commented May 9, 2016

Agree with feature attribute comment.
However it would make sense to add attribute after merging new changes to the compiler feature attributes from Future.
I do not want to make the merge a part of this change, so I will do another change for the attributes after the merge.

@VSadov VSadov merged commit a8880b9 into dotnet:features/generators May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants