search

projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.04k stars 95 forks source link

Fix bug 1491867: Add ability to wrap pseudolocale strings in markers #225

Closed mathjazz closed 2 years ago

mathjazz commented 3 years ago

@dminor r?

No idea why the tests are failing...

dminor commented 3 years ago

@dminor r?

No idea why the tests are failing...

The tests pass locally for me with your changes (both stable and nightly). It is concerning that the crash is occurring in the fluent-pseudo test cases.

zbraniecki commented 3 years ago

You don't have that many options and by the nature of this patch it is not critically performant so aiming for easy maintenance is the key, but if you wanted, two ways to "improve" the code could be:

The latter is probably what I would aim for but the difference is not important for this API. If you want you could look at how we handle PDI/FSI marks in fluent-bundle - https://github.com/projectfluent/fluent-rs/blob/master/fluent-bundle/src/resolver/pattern.rs#L62

I'd suggest adding more tests like - two literal parts separated by placeable, two literals side by side etc.

It would be also nice to consider if we want markers per string or per literal - @flodolo, @dminor , @mathjazz - up to you!

flodolo commented 3 years ago

It would be also nice to consider if we want markers per string or per literal - @flodolo, @dminor , @mathjazz - up to you!

Sorry, could you give me an example of the two to understand?

zbraniecki commented 3 years ago

Sorry, could you give me an example of the two to understand?

Sure! Example: key = Hello, { $userName } to our product!

Do we want [Hello, John to our product!] or maybe sometimes we may want to see [Hello, ][John][ to our product]?

Are message parts boundaries ever useful? Could they be to differentiate between key = You have 5 messages vs key = You have { $count} messages?

flodolo commented 3 years ago

Are message parts boundaries ever useful? Could they be to differentiate between key = You have 5 messages vs key = You have { $count} messages?

I think they risk being misleading? For example, would I be able to distinguish if Hello, ][John][ to our product] is the result of one string with a variable in the middle, or a collage of 3 strings?

On the other hand, it might be interesting to have something like [Hello, {John} to our product], assuming that's possible.

dminor commented 3 years ago

I think markers per string are fine for this PR.

Please do add some additional tests like Zibi suggested. You'll also need to bump the version number as part of this.

Since the tests only failed on Nightly, there's a chance the test failures were caused by the Nightly we were using when this was pushed. Unless we've pinned the Nightly version somewhere, in which case, we might want to try with a newer Nightly.

mathjazz commented 2 years ago

Quick update: transform() is called per PatternElement, so we can't really have per-Message markers easily.

That also means we can't have tests for literals separated by placeables- transform_dom() takes resolved strings as input (as opposed to fluent messages), so it can't really make a distinction between a literal and a placeable.

Are we OK with that or should we refactor how transform() works?

Screenshot 2021-07-16 at 17 51 43

/cc @dminor @zbraniecki @flodolo

mathjazz commented 2 years ago

Seems like the test is failing due to a bug in the coveralls version we use: https://github.com/nickmerwin/node-coveralls/issues/280

Should I address it here or in a followup?

Other than that and the question raised above, this is ready for another look. Once the new version of fluent-pseudo is released, I can open a patch for the Firefox implementation bug.

dminor commented 2 years ago

Seems like the test is failing due to a bug in the coveralls version we use: nickmerwin/node-coveralls#280

Should I address it here or in a followup?

Other than that and the question raised above, this is ready for another look. Once the new version of fluent-pseudo is released, I can open a patch for the Firefox implementation bug.

I think it would be best to keep the tests green, so please fix the coveralls version here as well. I'll be happy to approve this once that is done.

dminor commented 2 years ago

I'd also suggest we're fine with the current transform() behaviour, this is a step in the right direction, we can always iterate on it later.

mathjazz commented 2 years ago

After I've updated the Changelog, coveralls no longer breaks. I believe my diagnosis from above is false - this is not related to the coveralls version. (We use the Coveralls GitHub Action from the Marketplace.)

Since the tests are green now, and since there are several similar test failures related to coveralls on master, I think we should address that problem separaretlly. @dminor Thoughts?

flodolo commented 2 years ago

That also means we can't have tests for literals separated by placeables- transform_dom() takes resolved strings as input (as opposed to fluent messages), so it can't really make a distinction between a literal and a placeable.

Double checking: does it mean that we can't have different markers for messages and placeables (variables, term references)?

The screenshot look good to me, it'd be great if I could visually differentiate between two potentially concatenated strings and a placeable in the middle of a message, but that's OK if that turns out to be too complicated.

mathjazz commented 2 years ago

Double checking: does it mean that we can't have different markers for messages and placeables (variables, term references)?

Correct.

The Zibi's example (key = Hello, { $userName } to our product!) will result in [Hello, ][John][ to our product].

The screenshot look good to me, it'd be great if I could visually differentiate between two potentially concatenated strings and a placeable in the middle of a message, but that's OK if that turns out to be too complicated.

That's good to know! We can still iterate on it later.

dminor commented 2 years ago

After I've updated the Changelog, coveralls no longer breaks. I believe my diagnosis from above is false - this is not related to the coveralls version. (We use the Coveralls GitHub Action from the Marketplace.)

Since the tests are green now, and since there are several similar test failures related to coveralls on master, I think we should address that problem separaretlly. @dminor Thoughts?

Makes sense to me!