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

Replace PO parse / compose with Expo library #307

Merged
merged 3 commits into from Jul 20, 2022

Conversation

maennchen
Copy link
Member

Partial implementation of #215

Blocked by elixir-gettext/expo#30

This is only an initial draft and not the finished implementation. I'll be on vacation until 25th and will probably not work on this until then.

@maennchen
Copy link
Member Author

@whatyouhide I would really like to finish up this PR. Will you be able to have a look at the outstanding parser issue or should I try to find some help somewhere else?

@whatyouhide
Copy link
Contributor

@maennchen hey, sorry for the wait. I was spending my OSS cycles mostly on Xandra the past few weeks, and I have just a little more to go. I would be able to take a look at this in something like a couple of weeks, would that works? If I'll get to it sooner, I'll get to it sooner 😄

@maennchen
Copy link
Member Author

@whatyouhide Thanks for the feedback. Xandra sounds interesting, I'll have to look at that one a bit closer :)

I think I'm going to attempt it once more on my own in the meantime and if that doesn't work I'll wait for you.

@maennchen
Copy link
Member Author

@whatyouhide I tried in a new way and I nearly got it working.

To get to this point, I added a $end token and tried to get rid of as many $empty states as possible.

There are now only 2 errors left. It seems like the previous and obsolete tokens are somehow consumed prematurely.

Details: elixir-gettext/expo#30

@maennchen maennchen force-pushed the expo_integration_po_parser branch 4 times, most recently from 0d7a230 to 4805d5b Compare July 20, 2022 17:30
@maennchen
Copy link
Member Author

@josevalim FYI: I quickly rebased & fixed the tests.

I will have to do one more pass over the PR and make sure all the naming etc. is correct.

@maennchen maennchen force-pushed the expo_integration_po_parser branch 2 times, most recently from e131109 to 14f3bca Compare July 20, 2022 19:45
@maennchen
Copy link
Member Author

@josevalim / @whatyouhide I've completed the integration.

A few points to note:

  • Look at the commits separately
    • commit 1 is the actual integration
    • commit 2 renames translations to messages (except public function names etc.; as discussed with @josevalim via slack)
    • commit 2 has no actual code changes, it's all just names
  • Even though expo already supports previous / obsolete, that has no noticeable impact to the user of gettext
    • previous translations are kept on merge. no new ones are added
    • obsolete translations are discarded on compile -
    • obsolete translations are kept on merge. no new ones are added
    • since neither were supported and would've been broken by merge previously, I think it's a safe assumption that our users do not have any of those anyways
  • flags MapSet => nested lists
    • needed so that the file formatting does not change when merging po files (order & line separation)

Thank you both for your time :)

@maennchen maennchen marked this pull request as ready for review July 20, 2022 19:54
@@ -1,139 +1,176 @@
defmodule Gettext.MergerTest do
defmodule GettextMergerTest do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defmodule GettextMergerTest do
defmodule Gettext.MergerTest do

@josevalim josevalim merged commit d845020 into elixir-gettext:main Jul 20, 2022
@josevalim
Copy link
Contributor

💚 💙 💜 💛 ❤️

@maennchen maennchen deleted the expo_integration_po_parser branch July 20, 2022 20:20
@maennchen
Copy link
Member Author

@josevalim Oh wow, I was not expecting for this to get through that easy. Thanks ❤️

@josevalim
Copy link
Contributor

We just need an expo release and we can start working on the other issues. :)

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

3 participants