share-argument-exception-extension

[Zombach]

Description

message

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
    • Is this introducing a breaking change?
      • Yes
        • Link to aspire-docs issue (please use this breaking-change template):
      • No
        • Link to aspire-docs issue (please use this doc-idea template):
  • No

[Zombach]

@danmoseley Hello. Please look. Made a check share code

[danmoseley]

where you've added new checks, do they need tests?

[Zombach]

I will add today. I will also add tests to "Builder.addgarnet ..." => "Addganet (Builder ..."
I just wanted to understand, this option is suitable for the implementation of the Share check

[Zombach]

@danmoseley If anything is ready, you can fill.

[mitchdenny]

This looks good to me. Just need the covering tests for all the other resource types.

[danmoseley]

Tests should pass in all locales

[RussKie]

I think there are (at least) two ways of solving this.

1. Tactical - set the culture for each test and unset at it at the end. E.g. something like this pseudocode:

       try
       {
            _originalCulture = Thread.CurrentThread.CurrentCulture;
            _originalUICulture = Thread.CurrentThread.CurrentUICulture;
   
            CultureInfo.DefaultThreadCurrentCulture = Culture;
            Thread.CurrentThread.CurrentCulture = "en-US";
            Thread.CurrentThread.CurrentUICulture = "en-US"
   
           // ... body of the test
       }
       finally
       {
            Thread.CurrentThread.CurrentCulture = _originalCulture;
            Thread.CurrentThread.CurrentUICulture = _originalUICulture;
       }

2. Pass the culture as a theory argument. This would allow asserting the messages/formatting are correct across different locales.
3. Strategic - add an attribute which can be applies to a test/fixture. Something like dotnet/winforms@9a26e66 (though it'd be simpler since there are no unmanaged resources here).

[Zombach]

I think there are (at least) two ways of solving this.

1. Tactical - set the culture for each test and unset at it at the end. E.g. something like this pseudocode:

       try
       {
            _originalCulture = Thread.CurrentThread.CurrentCulture;
            _originalUICulture = Thread.CurrentThread.CurrentUICulture;
   
            CultureInfo.DefaultThreadCurrentCulture = Culture;
            Thread.CurrentThread.CurrentCulture = "en-US";
            Thread.CurrentThread.CurrentUICulture = "en-US"
   
           // ... body of the test
       }
       finally
       {
            Thread.CurrentThread.CurrentCulture = _originalCulture;
            Thread.CurrentThread.CurrentUICulture = _originalUICulture;
       }

2. Pass the culture as a theory argument. This would allow asserting the messages/formatting are correct across different locales.
3. Strategic - add an attribute which can be applies to a test/fixture. Something like dotnet/winforms@9a26e66 (though it'd be simpler since there are no unmanaged resources here).

I chose the 3rd point.
But this did not help this test, here is the problem with ''. ' => ',' which depends on OSPlatform
>

You need to understand what to do with it.
Also due to static resources when using
[UsedEfaultxunitculture]
This test breaks even more.

[danmoseley]

@Zombach not about this PR directly, but chatting offline, I think we want to hold off on any future large "argument validation" PR's without agreement first. Yes, code changes should validate arguments. A PR fixing a particular instance is probably fine. But any more large scale ones don't make sense right now as we're biasing for speed, and it brings the risk of regressions.

[Zombach]

@Zombach not about this PR directly, but chatting offline, I think we want to hold off on any future large "argument validation" PR's without agreement first. Yes, code changes should validate arguments. A PR fixing a particular instance is probably fine. But any more large scale ones don't make sense right now as we're biasing for speed, and it brings the risk of regressions.

Yes, I will make Culture fix in a separate PR.
And i will roll back this one to the previous state (before Culture fix)

[RussKie]

Instead of doing this in each project file, I suggest considering a different approach.

• If this file needs to be linked in each project, then do this injection at Directory.Build.props/targets (or alike).
• If this file needs to be conditionally linked in project, then do a conditional injection at Directory.Build.props/targets (or alike) controlled by an opt-in switch in each project.
  For example: https://github.com/dotnet/extensions/blob/1e4f6c51798be661b8d8743dd464ca9f54b0c9a3/src/Libraries/Microsoft.AspNetCore.HeaderParsing/Microsoft.AspNetCore.HeaderParsing.csproj#L15C6-L15C41

[RussKie]

Why not put the shared file in the Shared folder (like the line above)?

[RussKie]

These extensions aren't very discoverable. I very much suggest taking an inspiration from https://github.com/dotnet/extensions/tree/1e4f6c51798be661b8d8743dd464ca9f54b0c9a3/src/Shared/Throw, and do the same way (both in terms of API naming and the location of the file).

[Zombach]

Ooo. This option is really good. I will do the other day

[danmoseley]

Let's merge this after the 9.3 snap

[dotnet-policy-service]

This submission has been automatically marked as stale because it has been marked as requiring author action but has not had any activity for 14 days.
It will be closed if no further activity occurs within 7 days of this comment.