Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Culture Scope #7537

Merged
merged 10 commits into from
Nov 7, 2020
Merged

Introduce Culture Scope #7537

merged 10 commits into from
Nov 7, 2020

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Nov 6, 2020

It's related to the PR #7528 and my comments there

@hishamco hishamco marked this pull request as ready for review November 6, 2020 21:38
@hishamco hishamco requested a review from jtkech November 6, 2020 21:46

public CultureInfo Culture { get; }

public CultureInfo UICulture { get; }
Copy link
Member

Choose a reason for hiding this comment

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

@hishamco i will approve your PR

But just one thing, i don't see any usage of these 2 public properties

Copy link
Member Author

Choose a reason for hiding this comment

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

I point to where it's used, also we might to expose the original culture because may it's useful in some cases

using (var cultureScope = CultureScope.Create(culture))
{
// using DefaultPluralRuleProvider to test it returns correct rule
TryGetRuleFromDefaultPluralRuleProvider(cultureScope.Culture, out var rule);
Copy link
Member Author

@hishamco hishamco Nov 7, 2020

Choose a reason for hiding this comment

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

But just one thing, i don't see any usage of these 2 public properties

@jtkech it's used here

Copy link
Member

Choose a reason for hiding this comment

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

First, based on the removed line, here maybe cultureScope.UICulture

Okay, so you also use the culture scope as a CultureInfo helper to get a cultureInfo from a string

Then because the culture scope set the async local cultures you can just use CultureInfo.CurrentUICulture, that's the goal of this scope to set cultures that can be retrieved from everywhere called in the context of this scope

        using (CultureScope.Create(CultureInfo.GetCultureInfo(culture)))
        {
            // using DefaultPluralRuleProvider to test it returns correct rule
            TryGetRuleFromDefaultPluralRuleProvider(CultureInfo.CurrentUICulture, out var rule);

Or if you want to keep the helper part creating a cultureInfo from a string

        using (CultureScope.Create(culture))
        {
            // using DefaultPluralRuleProvider to test it returns correct rule
            TryGetRuleFromDefaultPluralRuleProvider(CultureInfo.CurrentUICulture, out var rule);

Copy link
Member Author

Choose a reason for hiding this comment

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

First, based on the removed line, here maybe cultureScope.UICulture

Ok, but it's the same in this case

Okay, so you also use the culture scope as a CultureInfo helper to get a cultureInfo from a string

There's already an overload to do that

Copy link
Member

Choose a reason for hiding this comment

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

Okay will see tomorrow if it is not yet merged ;)

The main thing is that you are using a scope to set the static async local CultureInfo properties, and then for your tests in place of retrieving these values from this same async local CultureInfo you are retrieving them from the scope you just created, so here you just test the properties of this scope, you are not testing what it is intended to do.

E.g. when using this scope for liquid rendering (when not in a real view context), the template rendering will still retrieve the cutures from the static CultureInfo, that's the goal

Copy link
Member Author

Choose a reason for hiding this comment

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

I may add some unit tests

@Skrypt Skrypt self-requested a review November 7, 2020 06:16
}

[Fact]
public void CultureScopeRetreivesTheOrginalCulturesIfExceptionOccur()
Copy link
Member Author

Choose a reason for hiding this comment

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

E.g. when using this scope for liquid rendering (when not in a real view context)

@jtkech no need to test a fake ILiquidTemplateManager this unit tests ensures if the Render throws an exception it will restore the original cultures

@agriffard agriffard merged commit d69e261 into dev Nov 7, 2020
@delete-merged-branch delete-merged-branch bot deleted the hishamco/culture-scope branch November 7, 2020 11:21
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