Skip to content

Conversation

@sharwell
Copy link

No description provided.

@sharwell sharwell marked this pull request as ready for review April 28, 2023 16:12
@sharwell sharwell requested a review from a team as a code owner April 28, 2023 16:12
@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #6614 (9e4a317) into main (2420507) will increase coverage by 0.00%.
The diff coverage is 93.47%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6614   +/-   ##
=======================================
  Coverage   96.40%   96.40%           
=======================================
  Files        1374     1374           
  Lines      320898   320911   +13     
  Branches    10390    10393    +3     
=======================================
+ Hits       309362   309379   +17     
+ Misses       9054     9051    -3     
+ Partials     2482     2481    -1     

/// </summary>
private readonly ConditionalWeakTable<AdditionalText, ApiData> _additionalTextToApiData = new();
private static readonly SourceTextValueProvider<ApiData> s_shippingApiDataProvider = new(static text => ReadApiData(text, isShippedApi: true));
private static readonly SourceTextValueProvider<ApiData> s_nonShippingApiDataProvider = new(static text => ReadApiData(text, isShippedApi: false));
Copy link
Member

Choose a reason for hiding this comment

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

i'd honestly prefer we just have AdditionalTextValueProvider

Copy link
Author

Choose a reason for hiding this comment

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

IMO this is a solid move forward for the analyzer in the known absence of that API. Also note that this analyzer references Roslyn 1.2.1 for maximum compatibility with downstream projects, so even if AdditionalTextValueProvider was added, it would potentially be problematic to adopt in the short term.

Copy link

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

LGTM. You'd want to follow-up with @CyrusNajmabadi to also get his confirmation.

@sharwell sharwell merged commit 31e5d27 into dotnet:main May 1, 2023
@sharwell sharwell deleted the public-api-cache branch May 1, 2023 16:22
@github-actions github-actions bot added this to the vNext milestone May 1, 2023
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.

3 participants