Skip to content

Conversation

@SimonCropp
Copy link
Contributor

No description provided.

@SimonCropp SimonCropp marked this pull request as draft April 10, 2025 10:08
@SimonCropp SimonCropp marked this pull request as ready for review April 10, 2025 11:54
@nohwnd nohwnd mentioned this pull request May 5, 2025
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

I am not a super big fan of having public types when it's not needed - we have too many issues and problems linked to public types where we don't know if they are used preventing us from doing any change.

I also think IVTs kinda suck but from source to test it feels ok to me - at least preferable than public.

@nohwnd
Copy link
Member

nohwnd commented May 12, 2025

Keeping internal, and replacing by #5561

@nohwnd nohwnd closed this May 12, 2025
@SimonCropp
Copy link
Contributor Author

@Evangelink but isnt this a source generator package, and hence not possible for people to reference its types as a public api

@Youssef1313
Copy link
Member

@Evangelink but isnt this a source generator package, and hence not possible for people to reference its types as a public api

It's technically possible to reference the types. But it requires adding an explicit reference in csproj similar to:

<Reference Include="$(NuGetPackageRoot)\package.name\package.version\analyzers\dotnet\cs\FileName.dll" />

It's extremely uncommon though, and I personally wouldn't consider it as a supported scenario. Still, I would like to keep IVT for tests once the work for polyfill EmbeddedAttribute is done. I haven't yet got to complete my PR though, but hopefully soon I'll complete it.

@SimonCropp SimonCropp deleted the remove-redudnant-InternalsVisibleTo-in-MSTest.SourceGeneration branch July 24, 2025 07:59
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