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

Refactor Culture and supported_cultures #16675

Closed
sebastienros opened this issue Sep 5, 2024 · 6 comments · Fixed by #16968
Closed

Refactor Culture and supported_cultures #16675

sebastienros opened this issue Sep 5, 2024 · 6 comments · Fixed by #16968
Milestone

Comments

@sebastienros
Copy link
Member

In this PR #16208

  1. Culture was introduced to represent the selected culture for the context. We should just be able to return a new ObjectValue with the current culture in it and not have to proxy every property since CultureInfo is already registered.
  2. supported_cultures should not be a filter, because it's not using the input. We could pass anything to it and it would still work. I suggest we register a FluidValue like SupportedCultures so we can write for cultures in SupportedCultures.
  3. We should find a better way or improve Fluid to be able to register values and resolve its properties with lambdas. Instead of creating a XXXFluidAccessor then register it, like Environment, Culture, ...
@sebastienros sebastienros added this to the 2.x milestone Sep 5, 2024
Copy link
Contributor

github-actions bot commented Sep 5, 2024

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

@agriffard agriffard changed the title Refactor Cutlure and supported_cultures Refactor Culture and supported_cultures Sep 10, 2024
@sebastienros
Copy link
Member Author

@hishamco Wondering if it would make sense to expose SupportedCultures like Culture.SupportedCultures instead. Would it feel more natural to have it under a common thing.

And another comment about something that might not work with the recent PR we merged, we should be able to pipeline SupportedCultures (in any form we decide to expose it) with the properties on a CultureInfo like Name, Dir, ... So we need to add a converter from CultureInfo to CultureValue, but one that saves its culture as a state, so what you had done initially ...

@hishamco
Copy link
Member

hishamco commented Nov 7, 2024

@hishamco Wondering if it would make sense to expose SupportedCultures like Culture.SupportedCultures instead. Would it feel more natural to have it under a common thing.

Could be, but it might be confusing to have supported cultures inside culture, what about locale.Culture, locale.SupportedCultures?

So we need to add a converter from CultureInfo to CultureValue, but one that saves its culture as a state, so what you had done initially ...

Let me start a new PR and ping you then

@hishamco
Copy link
Member

hishamco commented Nov 7, 2024

So we need to add a converter from CultureInfo to CultureValue

@sebastienros I'm not sure if it's doable now after you removed the CultureInfo from the CultureValue constructor

@sebastienros
Copy link
Member Author

Almost done on my side

@hishamco
Copy link
Member

hishamco commented Nov 7, 2024

I will wait for your PR, coz I already working on a new branch :) I will save the time and wait until you push your PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants