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

Expose [Explicit] to the world #1602

Merged
merged 4 commits into from
Oct 31, 2017
Merged

Expose [Explicit] to the world #1602

merged 4 commits into from
Oct 31, 2017

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Oct 24, 2017

Description

Fixes #1467

TODO

  • Changelog entry
  • Tests (if applicable)
  • Update PCL (if applicable)


private static bool ShouldInclude(ICustomAttributeProvider provider)
{
return provider.CustomAttributes.All(a => a.AttributeType.Name != "ExplicitAttribute");
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add an imported reference for ExplicitAttribute and compare against that. Otherwise we risk mistaking this for any other type with the same name but in a different namespace.

Comparing against the type name was part of the original implementation, but we ought to be safer now that this feature is public.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, and especially with Explicit being somewhat generic. Will update it.

@nirinchev nirinchev merged commit 8e2d8b3 into master Oct 31, 2017
@nirinchev nirinchev deleted the ni/explicit branch October 31, 2017 17:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants