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

Allow PublicAPI analyzers to merge from multiple sources #5422

Merged
merged 6 commits into from
Nov 2, 2021

Conversation

twsouthwick
Copy link
Member

@twsouthwick twsouthwick commented Aug 31, 2021

This changes updates the public change analyzer to merge the data about what is a public API from any additional file that follows the pattern PublicAPI.[Un]shipped.*txt. This allows, for example, for additional TFMs to add specific APIs they expose in an additive way. By default, APIs are still added to just the PublicAPI.[Un]shipped.txt file, but will be updated in the alternate files if needed.

Fixes #3187

This changes updates the public change analyzer to merge the data about what is a public API from any additional file that follows the pattern `PublicAPI.[Un]shipped.*txt`. This allows for additional TFMs to add specific APIs they expose in an additive way. By default, APIs are still added to just the `PublicAPI.[Un]shipped.txt` file, but will be updated in the alternate files if needed.
@twsouthwick twsouthwick requested a review from a team as a code owner August 31, 2021 04:22
@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #5422 (6b51a0d) into main (35483b7) will increase coverage by 0.01%.
The diff coverage is 97.07%.

@@            Coverage Diff             @@
##             main    #5422      +/-   ##
==========================================
+ Coverage   95.53%   95.55%   +0.01%     
==========================================
  Files        1275     1281       +6     
  Lines      292855   295979    +3124     
  Branches    17691    18047     +356     
==========================================
+ Hits       279783   282822    +3039     
- Misses      10654    10699      +45     
- Partials     2418     2458      +40     

@twsouthwick
Copy link
Member Author

twsouthwick commented Aug 31, 2021

dotnet/Open-XML-SDK#1011 is an example where I've built the PublicAPI docs around this change. You can see that the TFM specific ones are super small, and saves around 20mb from the duplication I'd have to do otherwise.

@twsouthwick
Copy link
Member Author

/cc @mavasani @Youssef1313 History indicates you've been involved on this analyzer

@jmarolf jmarolf self-assigned this Sep 1, 2021
@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 5422 in repo dotnet/roslyn-analyzers

@twsouthwick
Copy link
Member Author

@jmarolf I see you self-assigned this. Can you take a look? I'm readying a release of a library and would like to get it in to track changes for the next version if possible.

Copy link
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

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

looks great! thanks for working on this. I think we just need provide a codefix for each additional file that we have included in the project

@twsouthwick twsouthwick requested a review from jmarolf October 25, 2021 19:14
@twsouthwick
Copy link
Member Author

@jmarolf I had to change how equivalence keys were calculated and needed to encode the projectid/docid into it so that I could retrieve it in the fix all provider. If there's a better way to transmit that context let me know

@twsouthwick
Copy link
Member Author

@jmarolf I believe I've responded to all your feedback. Let me know if there's anything else I should do. Thanks!

@jmarolf jmarolf merged commit 3bf2b00 into dotnet:main Nov 2, 2021
@github-actions github-actions bot added this to the vNext milestone Nov 2, 2021
@twsouthwick twsouthwick deleted the merge-multiple-public-apis branch November 2, 2021 20:15
@twsouthwick
Copy link
Member Author

@jmarolf when is this anticipated to be released? Is there a CI feed this will be pushed to before made publicly available? On the README the link sends me to a feed that hasn't been updated for a while.

@jmarolf
Copy link
Contributor

jmarolf commented Nov 2, 2021

I need to update the readme. you can comsume these changes by creating a NuGet.config file that looks like this

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <clear />
    <add key="dotnet7" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7/nuget/v3/index.json" />
  </packageSources>
</configuration>

@jmarolf jmarolf modified the milestones: vNext, .NET 7.x Preview 1 Nov 3, 2021
@nohwnd
Copy link
Member

nohwnd commented Nov 11, 2021

Thanks for the change @twsouthwick ! I used it for vstest, if anyone is looking for more examples. We are in a bit specific situation where we support multiple tfms, including netstandard1.0 which has significant differences from the other tfms, so I have 2 big base files. 1 that is common to all tfms, and 1 that is common to all tfms apart from netstandard1.0, and then tfm specific files. I then use msbuild condition to combine them, and it works perfectly, with very little duplication:

common shipped + common net shipped + net451 shipped = shipped api for net451 tfm
common shipped + netstandard1.0 shipped = api for netstandard tfm

I am also including code in the PR comments that helped me split my original files into the common files:

microsoft/vstest#3165 (review)

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.

API checker needs to support multi-targeted projects
3 participants