Skip to content

Support Syntax 0.7 #76

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

Merged
merged 8 commits into from
Oct 23, 2018
Merged

Support Syntax 0.7 #76

merged 8 commits into from
Oct 23, 2018

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Oct 15, 2018

@stasm stasm mentioned this pull request Oct 15, 2018
ps.skip_inline_ws()

while ps.take_char(until_closing_bracket_or_eol):
pass
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR removes VariantName, we need a different way to skip 0.4 section titles. At first I thought about using a regex, but using the take_char method of FTLParseStream is more idiomatic to the rest of this file and also moves the cursor.

@stasm
Copy link
Contributor Author

stasm commented Oct 16, 2018

@Pike Do you want to review this? It's 99% simple port of the JS version, and 1% of 0.4 compatibility. I added a short readme in tests/syntax to keep track of fixtures which differ between JS and Python.

@Pike Pike self-requested a review October 16, 2018 20:07
@Pike
Copy link
Contributor

Pike commented Oct 16, 2018

I'll look into this.

@stasm
Copy link
Contributor Author

stasm commented Oct 17, 2018

Thanks. I have one more commit, a port of projectfluent/fluent.js@b09a623, which I'll push once you're done with the review. It moves things around a bit; it would make the review hard.

Copy link
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK, but we need a better solution for Windows line endings.

Seems that they don't work at all right now? Can we move that into a different issue/release?

@@ -30,15 +30,15 @@ def __init__(self, with_spans=True):
self.with_spans = with_spans

def parse(self, source):
ps = FTLParserStream(source)
ps.skip_blank_lines()
ps = FTLParserStream(source.replace('\r\n', '\n'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will invalidate all spans including error positions for files with Windows line endings.

Looking at the js side of things, that doesn't handle Windows line endings at all right now?

I think we should use the algorithm we have in the reference parser to fix this issue, i.e., normalize newlines on text node creation.

@@ -92,15 +92,15 @@ def parse_entry(self, source):
Preceding comments are ignored unless they contain syntax errors
themselves, in which case Junk for the invalid comment is returned.
"""
ps = FTLParserStream(source)
ps.skip_blank_lines()
ps = FTLParserStream(source.replace('\r\n', '\n'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

@stasm stasm requested a review from Pike October 18, 2018 19:11
@Pike
Copy link
Contributor

Pike commented Oct 19, 2018

I'd like to see the disambiguation between a failed takeChar and EOF in the python part, too.

The key lines would be

https://github.com/projectfluent/fluent.js/blob/61187c02ff8e7c08822ba7c828bf1aa9d7c4c42b/fluent-syntax/src/stream.js#L135-L142

which used to return undefined at the end and now return null. The python impl could return False, that might just work?

I ran the branch through compare-locales tests which pass, and also recreated a healthy amount of gecko-strings quarantine with the branch which resulted in the same hashes that flod generated with current master.

@stasm stasm merged commit 122c041 into projectfluent:master Oct 23, 2018
@stasm stasm deleted the zeroseven branch October 23, 2018 06:46
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.

3 participants