Skip to content

Conversation

@safern
Copy link
Member

@safern safern commented Jul 31, 2020

We noticed that a change in ApiCompat is causing ApiCompat to not check for difference in attributes. We're fixing that in: dotnet/arcade#5862

This reacts to that fix.

@safern safern requested review from Anipik and ericstj July 31, 2020 04:45
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

Interop looks good.

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Thanks, @safern. Sorry about the mismatches.

@safern
Copy link
Member Author

safern commented Jul 31, 2020

Sorry about the mismatches.

Oh that is hard to catch without the tooling. I just wanted to give you an FYI in case we needed to port this to P8 or just do that you know some APIs missed it in case someone reports that when trying the analyzer 😄

@safern
Copy link
Member Author

safern commented Aug 3, 2020

FYI: @eerhardt

@safern
Copy link
Member Author

safern commented Aug 3, 2020

Merging since failures are unrelated and pending pipelines are not affected by this change and I want to avoid merge conflicts or people adding attributes in other PRs and missing those and having to update this PR.

@safern safern merged commit a297222 into dotnet:master Aug 3, 2020
@safern safern deleted the UpdateAttributesDiff branch August 3, 2020 23:54
@adamsitnik
Copy link
Member

@safern thanks for catching and fixing all the issues! It's great that you have improved the tooling as well!

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
…#40185)

* Fix attribute differences in between refs and implementations

* Update APICompat version and address feedback

* Fix builds

* PR Feedback, include attributes for all tfms in S.T.Json ref
public partial class JsonConverterAttribute : System.Text.Json.Serialization.JsonAttribute
{
protected JsonConverterAttribute() { }
#if NETCOREAPP && !NETCOREAPP3_0
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: How come this was caught, but not the #if/def above

public static partial class JsonSerializer {
#if NETCOREAPP && !NETCOREAPP3_0
...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was just my mistake when looking at the diff and I accidentally discarded that change and then gave it a thought but forgot to redo the removal.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.