smhg / gettext-swig

Extract translatable strings from Swig template strings.
MIT License
1 stars 2 forks source link

Added context parsing and adjusted parser logic. #11

Closed eldarc closed 7 years ago

eldarc commented 7 years ago

This enables the use of context keywords. Example: {{ pgettext('title', 'A message to be translated') }} and {{ pgettext('article', 'A message to be translated') }} are registered as two messages.

However, xgettext-template has some problems when using --join-existing. It adds duplicates and doesn't actually join messages. xgettext-template also doesn't remove messages which are not anymore present in the source files (this is known from earlier).

smhg commented 7 years ago

Thank you for your efforts! First of all: can you make sure the tests pass? I'll have a look at the xgettext-template integration afterwards.

eldarc commented 7 years ago

Please review the changes done.

Regular and plural messages should work now with contexts.

smhg commented 7 years ago

Thank you!

eldarc commented 7 years ago
eldarc commented 7 years ago

Also, please keep in mind that I can't use the msgid as the key because there could be two identical messages that have different contexts.

smhg commented 7 years ago

Let's look at things one by one. Most of what you are doing was done in respect to Handlebars inside gettext-handlebars. I should have made this clear beforehand. I don't think you can reuse the actual code (besides, what you wrote is perfectly fine). But the input and output should be the same.

The keywordspec first. xgettext-template sends you an object like this:

  {
    _: {
      msgid: 0
    },
    gettext: {
      msgid: 0
    },
    dgettext: {
      msgid: 1
    },
    dcgettext: {
      msgid: 1
    },
    ngettext: {
      msgid: 0,
      msgid_plural: 1
    },
    dngettext: {
      msgid: 1,
      msgid_plural: 2
    },
    pgettext: {
      msgctxt: 0,
      msgid: 1
    },
    dpgettext: {
      msgctxt: 1,
      msgid: 2
    }
  }

There is really no need to accept a different format. With one exception: the old keywordspec format (when we didn't support context's yet). People who are using gettext-swig standalone benefit if we keep support for that. The old format is still in the Readme if you need a reference. Can you also please use the above as your default? This is gnu's xgettext default. It makes sense to stick to that.

One small note: xgettext-template actually sends you an array, extended with additional properties. Arrays are objects, so the result is completely the same as the above. This way both parsers which accept the old format and the ones which accept the new format are supported.

smhg commented 7 years ago

About the tests: template languages are very similar. Copying from the Handlebars parser's tests allows you to reuse the fixtures and test code you need. You'd basically only have to replace the syntax used in the fixtures. In this case, take fixtures/contexts.hbs and the accompanying tests. It is nicer if the context tests are separated. And this way we know input and output are the same.

smhg commented 7 years ago

Related to the keywordspec input, I'll summarize it better:

There is no need to parse an array of strings input just like xgettext-template. This parser, just like the other parsers, can be very strict about what it accepts as keywordspec input (the format above, and, preferrably, also the old format - see these lines in handlebars-gettext for how that is done).

xgettext-template takes care of providing this format (from the array of strings in this case). Let's not move this to the parsers, but keep it central.

Did I explain that properly?

eldarc commented 7 years ago

I think I got almost everything now.

I only removed the subexpression test. A bit more testing with the Regex is probably required for this to work. How should we proceed with this?

Also, are there any more known issues with the parser?

I tested this with xgettext-template and it still doesn't properly merge objects with the existing.

smhg commented 7 years ago

Thanks for all your work! It is hard to follow because I know next to nothing about Swig (I created this parser because it was so similar to how Handlebars was parsed in the early days). But if the tests work, then there is little room for issues. Is there a thing like subexpressions in Swig? Otherwise we don't need to worry about it.

About the merge with existing content: that's purely a xgettext-template issue, right? It works for all parsers or for none of them. If you can provide me a testcase (a new template and an existing po file), I'll have a look at it. Please put them in an new issue over at xgettext-template.

smhg commented 7 years ago

2 minor remarks:

eldarc commented 7 years ago

Thank you for your help! I appreciate it!

As I mentioned before I'm not interested in Swig. I'm going to use this as a starting point for a Nunjucks parser. I'll probably use the official Nunjucks parser instead of a Regex.

I'll work on that in the following days.

I feel like I'm done with Swig, it's not maintained (https://github.com/paularmstrong/swig).

Nunjucks is, however, a Mozilla package. I'm going to make a new project gettext-nunjucks and if you have time we can collaborate there?

eldarc commented 7 years ago

I hope the syntax is now ok?

Is there anything else?

smhg commented 7 years ago

Thank you for the changes. I will go through some tests with xgettext-template this week.

Related to Nunjucks: I would very much appreciate it if you'd set up a parser for it! Maybe start with the gettext-handlebars codebase? That is the 'reference' implementation for parsers. FWIW: you don't need to support the old keyword format in new parsers.