Skip to content

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jan 14, 2025

Contributes to #45928

  • Commit 1 adds the diagnostic message to Suppression, simplifies the ISuppressibleLog interface and its implementations and updates tests
  • Commit 2 allows specifying a suppressions culture when generating suppression files. It defaults to en-US. The switches are /p:ApiCompatSuppressionCulture (msbuild tasks property) and --suppression-culture (cli tool).

@ViktorHofer
Copy link
Member Author

cc @bitbonk

@ViktorHofer ViktorHofer self-assigned this Jan 14, 2025
@bitbonk
Copy link

bitbonk commented Jan 14, 2025

Which SDK(s) will this end up in? We are current on the latest .NET 8 SDK.

@ViktorHofer
Copy link
Member Author

Which SDK(s) will this end up in? We are current on the latest .NET 8 SDK.

I'm currently targeting main but I don't see a reason why this can't go into .NET 9 (9.0.200) which is the next feature release. We won't be able to backport this back to .NET 8 which is servicing only at this point.

Note that you can reference the Microsoft.DotNet.ApiCompat.Task NuGet package to consume features earlier. This change could be available in ~1-2 months on NuGet.org (I don't have the release schedule in front of me right now).

Note that this is an experimental PR, not ready to go in. I first need agreement from other owners of this tool.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

I'm supportive of adding the diagnostic message to the suppression file. I think it would help a lot. Some suggestions on how it's done.

Description = "If true, enables rule to check that the parameter names between public methods do not change.",
Recursive = true
};
CliOption<string> neutralLanguageOption = new("--neutral-language")
Copy link
Member

Choose a reason for hiding this comment

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

Naming: this was not intuitive to me. Consider something like --save-culture or similar. Ideally this setting won't affect other output of the application.

Copy link
Member Author

@ViktorHofer ViktorHofer Jan 14, 2025

Choose a reason for hiding this comment

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

The neutral language wording is based on the SDK option:

<AssemblyAttribute Include="System.Resources.NeutralResourcesLanguageAttribute" Condition="'$(NeutralLanguage)' != '' and '$(GenerateNeutralResourcesLanguageAttribute)' == 'true'">
<_Parameter1>$(NeutralLanguage)</_Parameter1>
</AssemblyAttribute>

What about --neutral-language-for-compatibility-suppressions (lol)?

Copy link
Member Author

@ViktorHofer ViktorHofer Jan 20, 2025

Choose a reason for hiding this comment

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

I changed it to --suppression-culture / /p:ApiCompatSuppressionCulture.

CultureInfo neutralLanguageCultureInfo = new(neutralLanguage);
Resources.Culture = neutralLanguageCultureInfo;
CommonResources.Culture = neutralLanguageCultureInfo;
PackageValidation.ResourceSingleton.ChangeCulture(neutralLanguageCultureInfo);
Copy link
Member

@ericstj ericstj Jan 14, 2025

Choose a reason for hiding this comment

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

It might be better to delay resource lookup until we write/render a diagnostic/suppression -- that way we could have a different resource lookup behavior for serialization vs UI. It looks like Roslyn has a similar scenario and uses LocalizableResourceString for that.

I can see how this singleton solution approximates that -- if we can assume that all resources will be serialized. Is this Resource used exclusively for holding diagnostics? I see some other resources in there -- maybe if we want to take this static approach you can split the resource types into one resx that's only used for diagnostics and another that's used for UI and add comments about that rule?

@github-actions
Copy link
Contributor

Due to lack of recent activity, this PR has been labeled as 'Stale'. It will be closed if no further activity occurs within 7 more days. Any new comment will remove the label.

@github-actions github-actions bot added the stale label Sep 22, 2025
@github-actions github-actions bot closed this Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants