Skip to content

Conversation

@drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Aug 25, 2024

These two utility classes should have the same members, with equivalent values. This change adds missing properties to both classes, and adds a test to ensure that they're kept in sync in future.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

These two utility classes should have the same members, with equivalent values. This change adds missing properties to both clases, and adds a test to ensure that they're kept in sync in future.
@drewnoakes drewnoakes added area-dashboard area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication testing ☑️ labels Aug 25, 2024
@drewnoakes drewnoakes added this to the 9.0 milestone Aug 25, 2024
@drewnoakes drewnoakes requested a review from JamesNK August 25, 2024 23:38
@drewnoakes drewnoakes requested a review from mitchdenny as a code owner August 25, 2024 23:38
@JamesNK
Copy link
Member

JamesNK commented Aug 26, 2024

What about using a type structure that forces them to be declared together? e.g.

public record KnownComparison(StringComparison Comparison, StringComparer Comparer);

public static class KnownStringFormats
{
    public static readonly KnownComparison ResourceName = new (StringComparison.OriginalIgnoreCase, StringComparer.OriginalIgnoreCase);
   // ... etc
}

// Usage
var resourcesByName = new Dictionary<string, ResourceViewModel>(KnownStringFormats.ResourceName.Comparer);

@drewnoakes
Copy link
Member Author

I think the test failure occurs when the current culture is also the invariant culture. Investigating.

What about using a type structure that forces them to be declared together?

This adds some runtime overhead and makes the call site's code more verbose, so I'm not a fan personally.

Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

This is a good idea.

@drewnoakes drewnoakes merged commit 25df905 into dotnet:main Aug 26, 2024
@drewnoakes drewnoakes deleted the drnoakes/string-comparers-sync-test branch August 26, 2024 02:59
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication area-dashboard testing ☑️

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants