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.

Sign up for GitHub

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

Jump to bottom

Fix #8520 - Component parameters not found #8811

Merged
merged 1 commit into from
Mar 27, 2019
Merged

Fix #8520 - Component parameters not found #8811

merged 1 commit into from
Mar 27, 2019

Conversation

rynowak
Copy link
Member

@rynowak rynowak commented Mar 26, 2019

This resolves the issue blocking use of component parameters from our
ref assemblies. Making properties public with private get is our
recommended guidance for wanting documentation to work in the IDE, and
it will be required when you ship ref assemblies - since ref assemblies
only contain public members.

@rynowak
Copy link
Member Author

rynowak commented Mar 26, 2019

Hmm looking at the ref assembly output, I'm not sure this will fix the problem. The setters aren't generated in ref assemblies.

@SteveSandersonMS
Copy link
Member

OK, this is inconvenient. It seems that internal set is also insufficient.

Maybe the correct fix is for us to change our parameter detection logic so we don't care whether there's a setter or not. As long as something is a property annotated with [Parameter], treat it as a parameter (and let it fail at runtime if there truly isn't a setter). That way it's OK for the reference assemblies not to have setters for those parameters.

The original reason for checking for the existence of set was because we didn't originally have [Parameter]. Now we do, the developer's intention is pretty clear.

Potential further improvement: We have an analyzer that currently errors if you make the setter public. We could change it so that it also errors if you have no setter at all, so nobody would get confused. This wouldn't affect the ref assemblies because we're not running the roslyn analyzer on those.

@rynowak rynowak requested a review from dougbu as a code owner March 26, 2019 16:25
@rynowak
Copy link
Member Author

rynowak commented Mar 26, 2019

Potential further improvement: We have an analyzer that currently errors if you make the setter public. We could change it so that it also errors if you have no setter at all, so nobody would get confused. This wouldn't affect the ref assemblies because we're not running the roslyn analyzer on those.

Or we would make the properties public set/get and add an analyzer to tell you not to set them.

I've updated this PR to include the setters in the ref assembly. Given that our nightly builds are broken (and have been for weeks) I don't really want to deliberate on the right solution until its working again. I think that this is an awful workaround, and I've logged #8825 to follow up on this issue.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 26, 2019
@rynowak
Copy link
Member Author

rynowak commented Mar 26, 2019

@SteveSandersonMS @pakrym

@pakrym
Copy link
Contributor

pakrym commented Mar 26, 2019

LGTM

@rynowak rynowak mentioned this pull request Mar 26, 2019
Open
@rynowak
Fix #8520 - Component parameters not found
b83ef78 
This resolves the issue blocking use of component parameters from our
ref assemblies. Making properties public with private get is our
recommended guidance for wanting documentation to work in the IDE.

We also now need to manually generate the ref-assembly types for these
so they will show up for tooling with setters. I've logged an issue to
track whether we want to keep this long term, it seems like a suitable
workaround for now.
SteveSandersonMS
SteveSandersonMS approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Great!

dougbu
dougbu approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

I'd like to understand why the project's properties are insufficient and when GenAPI.exclusions.txt changes will be needed going forward. But, that shouldn't block this PR…

@rynowak
Copy link
Member Author

rynowak commented Mar 26, 2019 •
edited
Loading

@aspnet/brt

@rynowak
Copy link
Member Author

rynowak commented Mar 27, 2019

I'd like to understand why the project's properties are insufficient and when GenAPI.exclusions.txt changes will be needed going forward. But, that shouldn't block this PR…

I'd be happy to chat about this in more detail in the office. Ultimately we don't want to maintain this long term, but we need a solution right now.

@rynowak
Copy link
Member Author

rynowak commented Mar 27, 2019

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=134101

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact ryanbrandenburg.

I've triaged the above build.

@rynowak rynowak merged commit cc1b294 into master Mar 27, 2019
@rynowak rynowak deleted the rynowak/parameter branch March 27, 2019 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbu dougbu dougbu approved these changes

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rynowak @SteveSandersonMS @pakrym @aspnet-hello @dougbu @Eilon