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

Issue #120: add support for markForI18n #122

Closed

Conversation

MarcVanDaele90
Copy link

@MarcVanDaele90 MarcVanDaele90 commented Nov 22, 2022

Thanks https://github.com/bauerj for the link. This was really useful.

First, I created a simple test case (which failed) and then I made the (indeed trivial) changes to make the test case succeed.

@bauerj
Copy link
Contributor

bauerj commented Nov 23, 2022

Hi @MarcVanDaele90,

thank you very much for raising this pull request. And congratulations for your first open-source contribution! 🥳

The code change looks good to me. But let's see what @marcglasberg thinks about it 🙂

You already added a test case, that's great! Can you also write a few sentences on how to use it for the README? Users probably also need to change their i18n.dart to include something like String get markForI18n => this;. This needs to be explained in the README.

@MarcVanDaele90
Copy link
Author

I think there is an issue with my fix/solution: If we just mark it with markForI18n, you don't know how it should be translated (normal or as plural) I guess, right?

@marcglasberg
Copy link
Owner

@MarcVanDaele90 @bauerj I don't have time to check this now, so what @bauerj decides is good for me.

@bauerj Please let me know if/when I should merge this.

@bauerj
Copy link
Contributor

bauerj commented Nov 25, 2022

I think there is an issue with my fix/solution: If we just mark it with markForI18n, you don't know how it should be translated (normal or as plural) I guess, right?

My understanding was that markForI18n simply keeps the original string so that:

"Apple".markForI18n == "Apple"
"Apple".markForI18n.plural(10) == "Äpfel"
"Apple".markForI18n.i18n == "Apfel"

Wouldn't this work?

@MarcVanDaele90
Copy link
Author

Well, I intended to use the markForI18n in my datalayer

objects = [{"name": "car".markForI18n, "image":"car.png"}, {"name": "apple".markForI18n, "image":"apple.png"}, ...]

and translate it when I use this object in the presentation layer (UI)

"How many %s do you see".i18n.fill([object.name.plural(100)]  //here I want to use the plural

But getstrings collects the strings from the markFor18n and there is no information that it will be used (later, in the UI) for plurals so the generated .pot file will not contain this information I guess.

I'll give it some more thoughts. I would suggest to ignore this 'fix' and to close issue #120 for now.

@bauerj
Copy link
Contributor

bauerj commented Nov 28, 2022

I see.

Indeed, this seems to render the proposed fix impossible 😞

Please close the PR then.

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.

None yet

4 participants