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.

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

Asm diff updates #13641

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

carlossanlop
Copy link
Member

Some fixes I've done over the few last months to address common problems in the API diff PRs. The main fix is related to the way we show APIs that had attribute changes.

No one else uses ApiDiff or Cci, that we know of, so please consider taking this change.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

@carlossanlop as your changes touch Cci.Extensions which is used by ApiCompat and GenAPI and I don't see any tests running for Cci.Extensions, can you please test your changes in places that use these tools? Presumably, AsmDiff is only used by you but CCI GenAPI is used in runtime and CCI ApiCompat is used in a few other repositories.

@missymessa
Copy link
Member

@carlossanlop are you still working on this?

@carlossanlop
Copy link
Member Author

@missymessa It's probably ready to merge, but it also needs a good review and making sure we don't break anything that consumes Cci that we're unaware of. But it's super low priority right now. My team will review it eventually.

Also, I occassionally add extra commits whenever I find bugs when running the api-diff tool.

Do you mind if we keep it open?

@markwilkie
Copy link
Member

Maybe merge right after GA?

@carlossanlop
Copy link
Member Author

Yes, we'll try to merge after GA. And if it's messing up with your metrics, I don't mind closing the PR and keep pushing my branch. When ready, I can create a new PR. Does that sound better?

@missymessa
Copy link
Member

Sounds good! Noticed this was a little stale of a PR, but it's not causing any concern with metrics. :)

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.

4 participants