Skip to content

Conversation

edwardneal
Copy link
Contributor

Description

At the moment, the build has a custom target, GenerateResourceStringsSource. This target has been around since the start of the repo; it runs prior to compile and generates a file which contains one const for each resource string. An example file is

internal partial class StringsHelper : Strings
{
    internal class ResourceNames
    {
        internal const string ADP_Ascending = @"Ascending";
        // ..etc.
    }
}

This ResourceNames class is only used in the ResCategory and ResDescription attributes applied to certain properties within the implementation assemblies for netfx and netcore.

This is redundant because the ResCategoryAttribute and ResDescriptionAttribute classes will already look up the parameter passed to them as a resource string. Switching to using this functionality (with associated test) means that we don't need the StringsHelper class, which means that we don't need the target which generates it.

Issues

None.

Testing

Two automated tests added to prove that the descriptions contain the localized values as we expect. One manual check by binding a PropertyGrid to a SqlConnectionStringBuilder (in netcore and netfx) to make sure that WinForms isn't performing something unusual to get the description in a way which bypasses the custom logic in ResCategoryAttribute and ResDescriptionAttribute.

@edwardneal edwardneal requested a review from a team as a code owner September 26, 2025 18:05
@paulmedynski paulmedynski self-assigned this Sep 26, 2025
@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@mdaigle mdaigle left a comment

Choose a reason for hiding this comment

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

Love it!

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Oct 2, 2025

This is actually used by Localization CI for generation of resource strings. Removing this target might break things. Would recommend taking a closer look for safety or adding necessary documentation around it to define its purpose.

@paulmedynski @mdaigle fyi for ref: Link to usage

Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

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

Needs careful observation into Localization workflow usage.

@benrr101
Copy link
Contributor

benrr101 commented Oct 3, 2025

I also love these changes, especially since my IDE complains about ResourceNames not existing a lot.
@cheenamalhotra is there any way we can validate the localization workflow prior to taking this PR?

@cheenamalhotra
Copy link
Member

I don't think there's an easy way to validate this before the merge - you can carefully scan for the targets being removed and what its doing from the logs.

or

Fail Fast: Go ahead and merge, be diligent to observe the Loc CI and be ready to revert this change if Localization CI is not happy after few days.

@edwardneal
Copy link
Contributor Author

Can the Localization CI run against a branch other than main? I'm happy to resubmit a PR which merges to another branch if that'd help validate the behaviour.

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.

5 participants