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

Added traits attributes to the generators tests #11182

Merged
merged 2 commits into from
May 10, 2016

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented May 9, 2016

Added traits attributes to the generators tests.
Some CR feedback tidying up.

@VSadov
Copy link
Member Author

VSadov commented May 9, 2016

@cston @jcouv @jaredpar @AlekseyTs - CR feedback on previous change. Primarily adding CompilerTrait attribute

using Roslyn.Test.Utilities;
using System.Linq;
using System.Threading;
Copy link
Member

Choose a reason for hiding this comment

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

I thought we want System usings to be on top (if the VS option is properly set).

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally do not care whichever way usings are sorted
I just use the built-in "Sort Using" feature.

If there is a discussion about this, I'd strongly support following the default behavior :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe we should care about the order at all.

@jcouv
Copy link
Member

jcouv commented May 9, 2016

LGTM
Can someone clarify what is our style for "usings" or, even better, can we automate it?

{
return CreateCompilationWithMscorlib(text, options: options, parseOptions: TestOptions.Regular.WithReplaceFeature());
}

[CompilerTrait(CompilerFeature.SourceGenerators)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we could simply mark the type and avoid the need to mark individual tests within it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's the much simpler approach. We should do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed now.

@AlekseyTs
Copy link
Contributor

LGTM, but consider marking the types as well (or instead) so that new tests wouldn't have to be marked.

@VSadov
Copy link
Member Author

VSadov commented May 10, 2016

I think I have addressed all concerns and concerns were minor.
So I will merge this and if anything new comes up we can fix that in later changes.

@VSadov VSadov merged commit ce2e2f0 into dotnet:features/generators May 10, 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.

5 participants