Skip to content

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

Merged
ericstj merged 1 commit into
dotnet:mainfrom
ericstj:apicompatComparePropEventAttr
May 2, 2022
Merged

Fix APICompat for attributes on properties, events, and enum values#9168
ericstj merged 1 commit into
dotnet:mainfrom
ericstj:apicompatComparePropEventAttr

Conversation

@ericstj

@ericstj ericstj commented Apr 22, 2022

Copy link
Copy Markdown
Member

This appears to have regressed with 45a923c

@ericstj
ericstj requested review from a team, ViktorHofer and terrajobst 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

ericstj commented Apr 25, 2022

Copy link
Copy Markdown
Member Author

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

@terrajobst

terrajobst commented Apr 25, 2022

Copy link
Copy Markdown

Oopsy. Thanks for root causing it.

@ericstj ericstj mentioned this pull request Apr 30, 2022
Closed
@ericstj

ericstj commented May 2, 2022

Copy link
Copy Markdown
Member Author

This is ready now, can I get a review?

@bartonjs

bartonjs commented May 2, 2022

Copy link
Copy Markdown
Member

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

ericstj commented May 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

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