Mock test for issue #213

[Pike]

This isn't a sound way to hook up this test, as it taints the upstream reference fixtures.

But I guess it'd be good way to give #213 a boost.

FWIW, I ran the crlf.ftl test locally, and it produced the expected results, though the tests fail.

This test fails in an unexpected way, in that it's not the TextElement that's segmented differently, but that the continuation isn't detected.

[zbraniecki]

Thanks to the test case, the fix was easy!

diff --git a/fluent-syntax/src/parser/pattern.rs b/fluent-syntax/src/parser/pattern.rs
index 84804b0..47b801b 100644
--- a/fluent-syntax/src/parser/pattern.rs
+++ b/fluent-syntax/src/parser/pattern.rs
@@ -75,7 +75,7 @@ where
                     }
                     let b = self.source.as_ref().as_bytes().get(self.ptr);
                     if indent == 0 {
-                        if b != Some(&b'\n') {
+                        if b != Some(&b'\r') && b != Some(&b'\n') {
                             break;
                         }
                     } else if !Self::is_byte_pattern_continuation(*b.unwrap()) {

Can you add this to your PR and we can merge it with the test?

[Pike]

I've filed an issue upstream. As per my comment, I think just putting additional fixtures in the rust repo isn't great.

Regarding your proposed fix, I wonder how that fares with pure cr mac-like files, so I extended the expected test change to do both.

As for the rust impl, I wonder if we should just hard-patch the fixture on the json side to enable the crlf test? That's still tainting the fixtures, but maybe in a more straight-forward way?

[zbraniecki]

Regarding your proposed fix, I wonder how that fares with pure cr mac-like files, so I extended the expected test change to do both.

You mean, just to prevent crashes, right? We don't support CR line ends: https://github.com/projectfluent/fluent/blob/master/spec/fluent.ebnf#L136

As for the rust impl, I wonder if we should just hard-patch the fixture on the json side to enable the crlf test? That's still tainting the fixtures, but maybe in a more straight-forward way?

I fixed that in our tests API just now. Which means that you can either make the test split \n or combine them the way reference fixtures do, and it'll work.

[zbraniecki]

Can you include my fix and rebase on top of master? This should be landable then.

[zbraniecki]

Oh, and with my fix, you can just extend the crlf.ftl file instead of introducing a new one! And upstream it :)

[Pike]

Oh, and with my fix, you can just extend the crlf.ftl file instead of introducing a new one! And upstream it :)

Right, that was where I was intending to go.

[zbraniecki]

Right, that was where I was intending to go.

Ok! In that case my proposal is:

1) I land my fix here 2) We land the fixture to test for it upstream 3) I vendor it in back here

How does it sound?

[zbraniecki]

@Pike can we close this?

[Pike]

I guess so. I left you a needinfo-ish thing upstream.