-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Following up on API baseline check and documentation #30221
Conversation
64dcb23
to
a2f5107
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly recommendations and suggestions (with a few nits for you to decide on) but I'd like to look again
docs/APIBaselines.md
Outdated
|
||
More information available in | ||
- https://github.com/dotnet/roslyn-analyzers/blob/master/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md | ||
- https://github.com/dotnet/roslyn-analyzers/blob/master/src/PublicApiAnalyzers/Microsoft.CodeAnalysis.PublicApiAnalyzers.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding a note to start at step (4) when making changes in projects with existing API baselines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need at least a note about the *REMOVED*
prefix whether or not the fixer will handle that automatically. Doubt other syntax notes are interesting to most people.
Speaking of special cases 😺 @BrennanConroy did the fixer handle most / all of the PublicAPI.Unshipped.txt changes in #29219 without manual changes, aside from any [SuppressMessage(...)]
attributes you needed❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I gave up on the fixer and did it all manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I remember the fixer doing a pretty good job even when fixing all instances of a problem in an entire (albeit filtered) solution. What went wrong e.g. is there an issue or three we should report in https://github.com/dotnet/roslyn-analyzers❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed inconsistent about which file it edited, so I couldn't just blindly accept whatever changes it wanted to make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, for example does it edit PublicAPI.Shipped.txt files rather than add *REMOVED*
lines in PublicAPI.Unshipped.txt❔
@sharwell and @mavasani is that a known issue❔ Could the fixer be fixed to always leave PublicAPI.Shipped.txt alone❔ (Well, except if you add a gesture for use after a release -- where the files should be merged.)
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest small touch-ups but I don't need to review again
Following up on feedback for #28253.