Skip to content

Fix APICompat for attributes on properties, events, and enum values #9168

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
Merged
merged 1 commit into from
May 2, 2022
Merged

Fix APICompat for attributes on properties, events, and enum values #9168

merged 1 commit into from
May 2, 2022

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Apr 22, 2022

This appears to have regressed with 45a923c

@ericstj ericstj requested review from terrajobst, ViktorHofer and a team April 22, 2022 20:44
@ericstj ericstj changed the title Fix APICompat on properties and events Fix APICompat for attributes on properties and events Apr 22, 2022
@ericstj ericstj changed the title Fix APICompat for attributes on properties and events Fix APICompat for attributes on properties, events, and enum values Apr 23, 2022
@ericstj ericstj mentioned this pull request Apr 23, 2022
Merged
@ericstj
Copy link
Member Author

ericstj commented Apr 25, 2022

Marking no merge until I fix all the fallout in runtime.

@terrajobst
Copy link

Oopsy. Thanks for root causing it.

@ericstj ericstj mentioned this pull request Apr 30, 2022
Closed
@ericstj
Copy link
Member Author

ericstj commented May 2, 2022

This is ready now, can I get a review?

@bartonjs
Copy link
Member

bartonjs commented May 2, 2022

The answer to all of my question may be "they'd've counted as different methods, and so it'd report as "missing+added", not "changed". But not knowing that for sure it seemed like reasonable questions to ask.

@ericstj
Copy link
Member Author

ericstj commented May 2, 2022 •
edited
Loading

The answer to all of my question may be "they'd've counted as different methods, and so it'd report as "missing+added", not "changed". But not knowing that for sure it seemed like reasonable questions to ask.

@bartonjs - can you check "hide whitespace" changes and re-review? You're reviewing code that didn't change.
image

The only thing this PR does is restores the code-path through this method for non-method-members (and adds a couple tests for it).

The PR that introduced the behavior your comments apply to was this: #7058
If you spot any issues with that could you file a separate issue, or perhaps propose a PR?

@ericstj ericstj merged commit 2c73d50 into dotnet:main May 2, 2022
@ericstj ericstj mentioned this pull request May 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@bartonjs bartonjs bartonjs approved these changes

@terrajobst terrajobst Awaiting requested review from terrajobst

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ericstj @terrajobst @bartonjs @ViktorHofer