-
Notifications
You must be signed in to change notification settings - Fork 28
Add the Placeable node #17
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
Conversation
dd9b56d
to
29bd96a
Compare
I'd like to open this up for your feedback, @zbraniecki, before I fix the tests and port this to fluent.js. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it a lot!
fluent/syntax/parser.py
Outdated
|
||
while ps.current(): | ||
ch = ps.current() | ||
|
||
if ch == '\n': | ||
break | ||
if not ps.is_peek_next_line_pattern(): | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zbraniecki This fixed the parsing of the following FTL:
key =
{ foo }
Bar
Before this change, get_pattern
would break
after the closing }
. Does this look right to you?
There's a bit of repetition between this code and get_text_element
. I wonder if that's OK? An alternative would be to make get_text_element
return None
in some cases so that we can break
in get_pattern
. However that would be inconsistent with the return values of other get_
methods, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think returning None
as an option is fine. If you really want to avoid that, it seems that returning a TextElement
with an empty buffer and then testing against the length of the buffer on the last element also works.
I think I prefer that over double is_peek_next_line_pattern
, but quite honestly, not by that much. If you find the duplication easier, I'm ok with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. After some more thought I decided to leave the second is_peek_next_line_pattern
in, with a comment explaining the end-condition of get_pattern
's loop.
Checking for length of the value of the last TextElement
would mean that we create the element, add spans to it in with_span
and then discard it. Returning None
would in turn create a precedent where we need to check the truthiness of nodes returned from get_
methods, at least in with_span
.
Another reason to not optimize this code wrt. duplication right now is that a lot of it might change when we fix #15. If it then turns out it's easier to return None
after all, we can add it.
04c3425
to
daa65c2
Compare
Let's move the review of this to projectfluent/fluent.js#64. Once it lands I'll port it to python. |
daa65c2
to
149d1b6
Compare
@zbraniecki Would you mind taking another look? It's an almost one-to-one port of the JS implementation, with the exception of the |
Hey @zbraniecki, can I assume this is good for landing since it's a close port of the JS implementation? |
yes, sorry! |
WIP. Implement projectfluent/fluent#52.
This doesn't pass structure tests because the AST changes, obviously. All but one behavior tests pass. I'm trying to find the bug.