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

Obsolete ResourceManagerWithCultureStringLocalizer and WithCulture interface member #3324

Closed
ryanbrandenburg opened this issue Jul 12, 2018 · 7 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates breaking-change This issue / pr will introduce a breaking change, when resolved / merged. Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@ryanbrandenburg
Copy link
Contributor

The ResourceManagerWithCultureStringLocalizer and WithCulture interface member are often sources of confusion for users of Localization, especially if they want to create their own IStringLocalizer implementation. These items give the user the impression that we expect an IStringLocalizer instance to be "per-language, per-resource", when actually they should only be "per-resource", with the language searched for determined by the CultureInfo.CurrentUICulture at execution time.

Neither of these has any usage that I was able to find with a cursory search, and it's difficult for me to imagine when they would be the correct option, so let's just get rid of them in 3.0.

@hishamco
Copy link
Member

Or we can move WithCulture as extension method as I mentioned here

@hishamco
Copy link
Member

@ryanbrandenburg can we mark them as Obsolete in release 2.2 branch and removing them in the master branch?

@ryanbrandenburg
Copy link
Contributor Author

Yeah, marking them as Obsolete would probably be step one, but this is just a suggestion on my part at this point. We'll have to talk it over with @mkArtakMSFT and probably @DamianEdwards to decide if they want this and what the plan would be first.

@hishamco
Copy link
Member

Sound good .. hope you check my earlier points in the issue that I referenced with them too ..

@hishamco
Copy link
Member

Another issue in WithCulture is if we passed a null it will create another instance with the same culture!!

@hishamco
Copy link
Member

hishamco commented Oct 6, 2018

@ryanbrandenburg any updates on this?

@Eilon Eilon added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed repo:Localization labels Nov 7, 2018
@mkArtakMSFT mkArtakMSFT added 1 - Ready enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Nov 7, 2018
@hishamco
Copy link
Member

@mkArtakMSFT any update on this? Can we mark the types as Obsolete? or shall we make a PR for 3.0.0?

@mkArtakMSFT mkArtakMSFT changed the title Remove ResourceManagerWithCultureStringLocalizer and WithCulture interface member Obsolete ResourceManagerWithCultureStringLocalizer and WithCulture interface member Feb 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates breaking-change This issue / pr will introduce a breaking change, when resolved / merged. Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

4 participants