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

Make CultureDictionary IEnumerable #6202

Merged
merged 2 commits into from
Dec 17, 2020

Conversation

hishamco
Copy link
Member

No description provided.

@hishamco hishamco force-pushed the hishamco/enumerable-culture-dictionary branch from 0302971 to 5eff7e5 Compare May 20, 2020 17:45
@hishamco hishamco added the ready label Aug 17, 2020
@hishamco
Copy link
Member Author

hishamco commented Dec 3, 2020

@sebastienros is it fine for you to merge this?

@hishamco hishamco requested a review from jtkech December 14, 2020 21:33
@agriffard
Copy link
Member

@jtkech Can you please review?

@jtkech
Copy link
Member

jtkech commented Dec 17, 2020

Okay will do soon ;)

@sebastienros
Copy link
Member

Not sure why you need it but it doesn't hurt

@agriffard agriffard merged commit 7d92ccd into dev Dec 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the hishamco/enumerable-culture-dictionary branch December 17, 2020 15:57
@jtkech
Copy link
Member

jtkech commented Dec 17, 2020

As i remember @hishamco was talking about perf but not sure ;)

@hishamco
Copy link
Member Author

Not about Perf @jtkech ;) that's another issue, the idea to make CultureDictionary foreachable which is useful in some cases

@jtkech
Copy link
Member

jtkech commented Dec 17, 2020

Okay cool then

Yes because i didn't see any current usage in the code

@hishamco
Copy link
Member Author

hishamco commented Dec 17, 2020

Perhaps there's no use case in the source at that time but I need to check now. If there's not at least you can iterate over this collection like the other once

@sebastienros
Copy link
Member

Perhaps there's no use case

I asked because this library can be used outside of OC, so I assumed there was some use case. But if you don't even have one then don't do anything. The only thing you can do is introduce bugs, or create more public surface to maintain.

@hishamco
Copy link
Member Author

Seem I made a typo in my last reply, what I mean is perhaps there's no use case in the source code at that time but there are some use cases like:

  • Po Parser could return a CultureDictionary

  • Filter the translations in CultureDictionary , this could be useful if I want to provide a UI for translation per component or view

there could be more use cases outside OC

@sebastienros
Copy link
Member

Ok, and this change doesn't seem risky. But it was good to talk about when to not do anything.

@hishamco
Copy link
Member Author

You are right .. at the time of writing the PR (7 months ago) there's a case where I need to traverse the CultureDictionary records I find there's no proper way to do that ;)

I think making PoParser returns CultureDictionary will be a proof / use case for this PR if you agreed with

Thanks

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.

4 participants