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

internal/xgettext, cmd/xgettext-go: add an xgettext-go command #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jhenstridge
Copy link

This is based on the xgettext-go utility in the snapd tree with some refinements. The main improvements are:

  1. Multiple keywords can be specified via -k/--keyword. This uses the same syntax as GNU xgettext to declare plural or context keywords, so e.g. --keyword-plural=i18n.DG would now be written as --keyword=i18n.DG:1,2. As with normal xgettext, the builtin keywords can be disabled by passing -k without an argument.

  2. Multiple comment tags can be registered with -c/--add-comments. Used without a value, all comments before translatable strings are collected.

  3. We use strconv.Unquote to decode string constants and strconv.Quote to quote them for inclusion in the template. This simplifies the handling, and means we correctly handle string constants like "hello "+`world` .

@m-horky
Copy link

m-horky commented Dec 7, 2023

Hi. We would like to start using this library, including this command. Is there anything I can do to help revive this PR and get it merged?
cc @jhenstridge

@m-horky
Copy link

m-horky commented Jan 30, 2024

Ping again. Is there anything we can do to help you with this PR or maintenance in general?

This is loosely based on the xgettext-go from snapd, extracting the
logic into a package and extending the keyword support to handle
unqualified keywords, along with plural and context keywords.
@jhenstridge
Copy link
Author

@m-horky: we're still using the package, I just didn't get round to chasing up reviews. We'd kind of hoped that the original package we'd forked from would accept some of the other changes, but that hasn't happened so I guess we should treat this as the upstream.

I've updated this PR to fix up the conflicts, and will look at getting it reviewed. If you've got the time, feel free to give your own review feedback. I'll look at getting the other PRs updated.

Copy link

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

Thank you.

I've went through the code and I didn't find anything that would stand out. I've tried it out on our code and it all works as expected.
I have left one note about using absolute paths; I think that may be a non-blocking comment that can be addressed later (I might try to find some time for that if needed). (I passed in absolute paths through -f {file}, my bad.)

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