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

yaml: Add references #117

Merged
merged 1 commit into from
May 26, 2018
Merged

yaml: Add references #117

merged 1 commit into from
May 26, 2018

Conversation

l2dy
Copy link
Contributor

@l2dy l2dy commented May 26, 2018

This PR adds references from the yaml hierarchy.

I'll fix the tests if it passes review.

@mquinson
Copy link
Owner

Hello. This seems pretty good, but I definitely need more tests to see what it really does. It's hard to tell as is.

Many thanks in advance,
Mt

@mquinson
Copy link
Owner

just to make it clear: i'd really appreciate if you could fix the existing tests, but also add some new tests that explore the intended behavior of your code.

Thx

@l2dy
Copy link
Contributor Author

l2dy commented May 26, 2018

I have updated the existing tests and I believe they already explore the intended behavior of this change.

@mquinson mquinson merged commit f30594d into mquinson:master May 26, 2018
@mquinson
Copy link
Owner

thanks, that's in :)

@l2dy l2dy deleted the yaml-ref branch May 26, 2018 11:02
@l2dy
Copy link
Contributor Author

l2dy commented May 26, 2018

Hi @mquinson,

Do you think > would be better than : since no line numbers are involved here?

@mquinson
Copy link
Owner

Hello @l2dy. I was wondering whether / would better denote the hierarchy idea. Not such a big deal, but you are free to change it if you prefer it this way.

More concerning is the fact that the reference is sometimes dupplicated:

#. type: Hash Value - Key: Name
#: :Topics::Topics: :Topics::Topics:
#, no-wrap
msgid "Troubleshooting"
msgstr ""

Is it really the expected behavior?

Thx

@l2dy
Copy link
Contributor Author

l2dy commented May 26, 2018

YAML::Tiny doesn't preserve ordering, so we can't rely on filenames and line numbers which are mostly unique for each entry.

But what happens if GNU gettext's xgettext(1) sees multiple identical strings on the same line? The references are deduplicated[1]! So IMHO, duplication is a problem shared across formats and deduplication shouldn't be done in the format modules.

Now, I don't have the Perl skills required to implement deduplication, but I'd say that the yaml module can stay as is regarding this. Mini test case in [2] for anyone willing to implementing deduplication.

[1]

  printf("\n%s\n", _("Compile options:"));printf("\n%s\n", _("Compile options:"));
#: version.c:404
msgid "Compile options:"
msgstr ""

[2]

a:
  - a
  - a
  - a
#. type: Array Element
#: :a :a :a
#, no-wrap
msgid "a"
msgstr ""

Edit: multiple identical strings

@mquinson
Copy link
Owner

Ah, I think I got it. In my previous example, the string "Troubleshooting" appears in two differing contexts, that happen to have the same reference in the tree. Am I right?

If so, we can definitely live with it. Thanks for your help.

@l2dy
Copy link
Contributor Author

l2dy commented May 26, 2018

I'm not sure why this module treat hash keys as part of the type, but at least for arrays they share the same reference (YAML::Tiny doesn't preserve ordering).

@mquinson
Copy link
Owner

I just implemented the reference deduplication in po4a (my plane is in 3 hours, I'm bored), but this does not change anything in yaml example files. I'm thus suspecting that the problem is elsewhere.

Not a big deal, but it's surprising, isn't it?

@l2dy
Copy link
Contributor Author

l2dy commented May 26, 2018

this does not change anything in yaml example files

No, it does. I'll make a PR soon.

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.

2 participants