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

Use RequestLocalizationOptions.CultureInfoUseUserOverride #14716

Merged
merged 4 commits into from
Dec 3, 2023

Conversation

hishamco
Copy link
Member

@hishamco hishamco requested a review from jtkech November 17, 2023 16:25
@jtkech
Copy link
Member

jtkech commented Nov 18, 2023

Okay looks good, I will approve.

But please only merge if you have retried it.

Hmm, I think we can just remove LocalizationOptionsUpdater but as you want.

Good job!

@hishamco
Copy link
Member Author

Hmm, I think we can just remove LocalizationOptionsUpdater but as you want.

I'm not sure if we can remove it now or not, it'll be a breaking change or shall we remove it because it's only used inside the module and the release note is enough

/// Initializes a new instance of a <see cref="LocalizationOptionsUpdater"/>.
/// </summary>
/// <param name="options">The <see cref="RequestLocalizationOptions"/>.</param>
public LocalizationOptionsUpdater(RequestLocalizationOptions options)
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems there's no need for the LocalizationOptionsUpdater changes, right? I did it before I rid off the use of LocalizationOptionsUpdater

@jtkech
Copy link
Member

jtkech commented Dec 3, 2023

We should merge this one.

I'm not sure if we can remove it now or not, it'll be a breaking change or shall we remove it because it's only used inside the module and the release note is enough

Yes I think you can just remove LocalizationOptionsUpdater, then I will merge.

As a side note, would have been less confusing to have a config value with the same name and same logic as UseUserOverride, I think we used IgnoreSystemSettings to have a default value of false, but it's possible to read a boolean from the config and having a default value of true if not specified.

@jtkech jtkech merged commit 50c8954 into main Dec 3, 2023
3 checks passed
@jtkech jtkech deleted the hishamco/localization-settings branch December 3, 2023 18:59
@jtkech
Copy link
Member

jtkech commented Dec 3, 2023

@hishamco

I removed LocalizationOptionsUpdater and merged your PR.

Notice that I didn't add myself as a co-author ;)

@hishamco
Copy link
Member Author

hishamco commented Dec 3, 2023

Believe it or not, the co-author it doesn't matter to me :) it seems next time I will open my eyes during the merge to avoid such an issue :)

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.

2 participants