Skip to content

ApiDiff: Add AssemblySymbolLoader method, a package dependency from CodeAnalysis, and minor fixes#45933

Closed
carlossanlop wants to merge 7 commits intodotnet:mainfrom
carlossanlop:ApiDiffPart5
Closed

ApiDiff: Add AssemblySymbolLoader method, a package dependency from CodeAnalysis, and minor fixes#45933
carlossanlop wants to merge 7 commits intodotnet:mainfrom
carlossanlop:ApiDiffPart5

Conversation

@carlossanlop
Copy link
Copy Markdown
Contributor

This PR is part of the work needed to create an ApiDiff tool that reuses some of the code from Microsoft.DotNet.GenAPI. The idea is to make the larger PR smaller and make it easier to review: #45389

The purpose of this change is to:

  • Add the dependency to Microsoft.CodeAnalysis.Workspaces.Common, as it will be used in the upcoming change. The files adding this dependency are often showing merge conflicts due to the packages above and below changing often, making it very difficult to keep the PRs up to date.
  • Add an API that will be later be used to refactor some of the other methods in the same class.
  • Some minor fixes of comments and typos.

@carlossanlop carlossanlop self-assigned this Jan 13, 2025
@carlossanlop carlossanlop requested review from a team as code owners January 13, 2025 18:49
@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Jan 13, 2025
Copy link
Copy Markdown
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.

Can you please describe why the new AssemblySymbolLoader API is necessary?

@carlossanlop
Copy link
Copy Markdown
Contributor Author

Can you please describe why the new AssemblySymbolLoader API is necessary?

Sure: In the original PR I added a factory method to create an AssemblySymbolLoader instance. It uses this LoadAssembliesAsDictionary method. I added this factory method to the PR.

But I also noticed that I have simplified the ApiDiff tool enough to not need this method anywhere else. So I'll make it private, as I still need it for the factory method.

@ViktorHofer
Copy link
Copy Markdown
Member

I was talking about this new public static method: public static (AssemblySymbolLoader, Dictionary<string, IAssemblySymbol>) CreateFromFiles(ILog logger, string[] assembliesPaths, string[]? assemblyReferencesPaths, bool respectInternals = false).

I would like to understand why AsmDiff needs a new entry point to AssemblySymbolLoader given that APICompat and GenAPI don't require it.

@carlossanlop
Copy link
Copy Markdown
Contributor Author

I'll split this. Some things can be sent separately alongside a bigger change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Infrastructure untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants