Skip to content

Require public component parameters and remove private metadata workaround #915

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

Merged
merged 1 commit into from
Jul 31, 2019

Conversation

NTaylorMullen
Copy link

  • Prior to this a user could have private, protected or public parameters. We now require public and plan to update analyzers to enforce good usage of public component parameters.
  • Removed private ComponentTagHelperDescriptorProvider workaround. meta data workaround since we no longer operate on private parameters.
  • Updated existing tests to not have private parameter setters. I missed these in my last pass.
  • Added two new tests to validate parameters which are private or have private setters are ignored.

dotnet/aspnetcore#12291

…round

- Prior to this a user could have private, protected or public parameters. We now require public and plan to update analyzers to enforce good usage of public component parameters.
- Removed private `ComponentTagHelperDescriptorProvider` workaround. meta data workaround since we no longer operate on private parameters.
- Updated existing tests to not have private parameter setters. I missed these in my last pass.
- Added two new tests to validate parameters which are private or have private setters are ignored.

dotnet/aspnetcore#12291
@@ -333,6 +317,7 @@ private void CreateContextParameter(TagHelperDescriptorBuilder builder, string c
// a dictionary keyed on property name.
//
// We consider parameters to be defined by properties satisfying all of the following:
// - are public
Copy link
Author

@NTaylorMullen NTaylorMullen Jul 30, 2019

Choose a reason for hiding this comment

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

Requiring public wasn't initially mentioned in the component visibility issue but it felt "more right" to me in an effort to default to being more restrictive for RTM to avoid edge cases. Not to mention it'll be an analyzer error by default. Let me know if you feel differently.

Copy link
Member

Choose a reason for hiding this comment

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

sure

@NTaylorMullen NTaylorMullen merged commit bb543aa into release/3.0 Jul 31, 2019
@ghost ghost deleted the nimullen/12291 branch July 31, 2019 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants