-
Notifications
You must be signed in to change notification settings - Fork 28
0.9 port, fluent.js -> python-fluent #113
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
Needs parser fixes to actually run.
Tests are green, requesting review. |
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.
Leaving some comments that I remember from porting the js impl over.
The fixtures are unmodified, as expected.
if isinstance(expression, ast.SelectExpression): | ||
return serialize_select_expression(expression) | ||
if isinstance(expression, ast.Placeable): | ||
return serialize_placeable(expression) | ||
raise Exception('Unknown expression type: {}'.format(type(expression))) | ||
|
||
|
||
def serialize_reference_expression(expression, id=None): |
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'm not tooooooo fond of this one, as it indicates a type weakness, I think. I've kept this in sync with the js impl, but there's no backing for this factorization in the the types in the AST.
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 for bringing it up. I'll fix this in fluent.js
and I'll follow-up in the PR with a port of the fix.
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 fixed this in fluent.js
in projectfluent/fluent.js#351 and I ported the fix over to this PR in 07f762f.
|
||
class MessageReference(Expression): | ||
def __init__(self, id, **kwargs): | ||
def __init__(self, id, attribute=None, **kwargs): |
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.
In the js impl, the names here are still attr
, but with actual keyword arguments in python, these need to match the properties for traverse
to continue to work.
self.positional = positional or [] | ||
self.named = named or [] | ||
class CallArguments(SyntaxNode): | ||
def __init__(self, positional=None, named=None, **kwargs): |
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 didn't want to put []
into default arguments, as it's non-pythonic.
@@ -60,7 +60,7 @@ def get_error_message(code, args): | |||
if code == 'E0025': | |||
return 'Unknown escape sequence: \\{}.'.format(args[0]) | |||
if code == 'E0026': | |||
return 'Invalid Unicode escape sequence: {}'.format(args[0]) | |||
return 'Invalid Unicode escape sequence: {}.'.format(args[0]) |
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.
This was a difference to js, which triggered structure failures on ex-behavior tests.
@@ -0,0 +1,145 @@ | |||
# coding: utf-8 |
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.
Note on this test, it's heavily inspired by the js tests, but I took a shortcut in the fixtures in a place or two.
Also, the U{6}
tests don't work on standard python2, as it doesn't have wide unichar support, so I made that depend on sys.
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, @Pike, I appreciate your help with this. I'd like to add a small fix to the serializer to this PR, so let's hold on with the landing for now.
if isinstance(expression, ast.SelectExpression): | ||
return serialize_select_expression(expression) | ||
if isinstance(expression, ast.Placeable): | ||
return serialize_placeable(expression) | ||
raise Exception('Unknown expression type: {}'.format(type(expression))) | ||
|
||
|
||
def serialize_reference_expression(expression, id=None): |
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 for bringing it up. I'll fix this in fluent.js
and I'll follow-up in the PR with a port of the fix.
This is just
fluent.syntax
.CHANGELOG should be done on a follow-up commit on the to-be-created zeronine branch, I think.