Closed gemmaro closed 3 weeks ago
Hello @gemmaro,
thanks for this bug report and patch. Please note that pull requests on github are much more comfortable for me than raw patches in the text. For once, I get an automatic testing of your change on all existing tests. If your patch seems reasonable and if all tests pass, I'm much more confident in integrating it right away.
To make sure that I merge your changes without a glitch, you should also add a test showcasing the bug that you are fixing, or a few tests of the new feature that you are adding + a note in the changelog explaining your change.
That being said, I unfortunately have a more deep concern with your patch. It is too wide as it quotes lists. It seems to me that the following yaml extract should be left unchanged by po4a:
button: "[ copy ]"
other: [ hello ]
The existing code removes the quote of the first line while your version quotes the second line. I'm not sure of how we could solve it. I always fear that we have to rewrite the parser ourselves and that this time we cannot keep using the parser that we are currently using. I'll try to think about it, but if you have any idea please speak up.
Hello @mquinson,
thanks for this bug report and patch. Please note that pull requests on github are much more comfortable for me than raw patches in the text. For once, I get an automatic testing of your change on all existing tests. If your patch seems reasonable and if all tests pass, I'm much more confident in integrating it right away.
To make sure that I merge your changes without a glitch, you should also add a test showcasing the bug that you are fixing, or a few tests of the new feature that you are adding + a note in the changelog explaining your change.
Thank you for the response. I will create pull requests (rather than pasting diffs) from now on.
That being said, I unfortunately have a more deep concern with your patch. It is too wide as it quotes lists. It seems to me that the following yaml extract should be left unchanged by po4a:
button: "[ copy ]" other: [ hello ]
The existing code removes the quote of the first line while your version quotes the second line. I'm not sure of how we could solve it. I always fear that we have to rewrite the parser ourselves and that this time we cannot keep using the parser that we are currently using. I'll try to think about it, but if you have any idea please speak up.
I found that the documentation of YAML::Tiny, used in po4a, says that "Support for flow-style sequences is not required." so it is expected behavior that other: [ hello ]
is not interpreted as an array. Due to the limitations of YAML::Tiny, both cases cannot be supported on the po4a side; the output is unified as either quoted or unquoted output. In summary,
original data | parsed result with YAML::Tiny | normalized result with po4a |
---|---|---|
hello: '[ world ]' |
hello: '[ world ]' |
hello: [ world ] |
hello: [ world ] |
hello: '[ world ]' |
hello: [ world ] |
Since po4a's documentation states that YAML files are to be parsed using YAML::Tiny, it seems to me that the preferred output is quoted one, and it is not a bug but a limitation in that case. It is possible that existing translations may be affected, but this can be handled by pre-processing on the user side. For example,
then it would yield a simpler YAML file. After doing so, po4a (and YAML::Tiny) can manage the translation without changing the original structure.
I think there are two options. For the second of these, I'd like to create a pull request with additional test cases later.
Use YAML libraries that support richer syntax
There might be alternative libraries with more extensive YAML syntax support. However, they might have issues such as increased memory usage or portability concerns that require C bindings.
Match po4a behavior as YAML::Tiny
This is the way presented in the draft patch in this issue. Both flow-style arrays and array-like strings are treated as strings. For the former, the workaround described above can be applied.
The problem is that some people rely on the fact that po4a leaves YAML lists unchanged. This is why the Locale::Po4a::Text module provides a yfm_skip_array option, to not translate the elements of the list. I can't remember directly but maybe f-droid webpages rely on this.
I guess that we have to go for the first solution, or even better: use our own YAML library (or use a modified version of YAML::Tiny that would be distributed as part of the po4a source code)
I start to think that I'm completely mislead on this bug... I was trying to support the following syntax to specify a list of two elements:
- hashvalue: [ "list elem1", "list elem2"]
But the thing is that YAML:Tiny does not support this. It is parsed as a string, explaining why we fail detecting that it's a list (that should not be quoted) in po4a. This limitation of YAML::Tiny is documented: https://metacpan.org/pod/YAML::Tiny#YAML-TINY-SPECIFICATION :
Support for the "[" flow sequence indicator is not required, with the exception of empty arrays (detailed below).
So I guess you are right, we should quote all strings, even the ones beginning with [
, even if it breaks some user code.
I almost have a working patch, but I must cook right now. Stay tuned.
I appreciate your handling of this matter. Wishing you a happy new year!
@mquinson running into problems with this change, and im not sure exactly where my issue is.
I'm not 100% certain where the problem is but my best guess is my issue is with kramdown in jekyll not removing/stripping valid* front matter.
original front matter, no problems here:
---
entry: "Merge Mining"
terms: ["merge-mine", "merge-mining", "merged-mining","merge-mined"]
summary: "The process of mining two or more blockchains at the same time"
---
po4a
, does not like in line lists anymore so i try:
---
entry: "Merge Mining"
terms:
- "merge-mine"
- "merge-mining"
- "merged-mining"
- "merge-mined"
summary: "The process of mining two or more blockchains at the same time"
---
my first wrinkle occured when i noticed the "---" change colour in my text editor indicating something was amiss with the front matter, but ok, ignoring that and building the site is ok, however, the front matter displays on the web page. whereas the original, "one liner" does not.
@plowsof yamllint reports the following issue on your file.
3:7 error trailing spaces (trailing-spaces)
Does removing the extra space after terms:
solve your problem?
thanks for the quick suggestion, sadly the problem remains. im leaning toward this being a jekyll / kramdown converter issue
if this is indeed a jekyll/kramdown issue i think we can close this and i create the issue there. here is a preview of the front matter now displaying: https://deploy-preview-14--effulgent-bombolone-f6cfb4.netlify.app/resources/moneropedia/merge-mining from https://github.com/plowsof/monero-site/pull/14/files
Please try using a yaml linter on your file to see whether there is another issue elsewhere. You really want the "---" changing colour again in the text editor, showing that the syntax error is fixed.
I have opened an issue on Jekyll with more information. Because it appears as though the yaml is correct, and values are being parsed from the list correctly, that this issue is jekylls problem. this issue can be closed if you agree, here is the issue, and much thanks for the quick responses: https://github.com/jekyll/jekyll/issues/9618
Ok. I close this issue then. On need, please open another issue with all details, and a reference to the current one.
Array like string literal like
[ bbb ]
is generated as[ bbb ]
, not as'[ bbb ]'
. Here is a test case:And the test results are the following:
I wonder if
lib/Locale/Po4a/TransTractor.pm
is the cause? The patch bellow seems to resolve this case (though there might be counter-examples):po4a version: 0.69.