Update fluent.runtime for fluent.syntax 0.10 #88

spookylukey · 2019-01-28T08:55:12Z

Fixes #86

…ures.

Pike

I gave this a bit of a read, but I'd prefer stas to make the actual review.

Some notes: I did check for some performance problems that I spotted on the compiler branch (the args check in resolve), that's not here, so that's good :-)

I prefer no else after return. Sometimes the code does it, sometimes not, let's be consistent about that.

I'd prefer the context manager and the sub-state to be explicit about "Variables in TermReferences". Make it non-generic? Took me a while to understand that that is what it's doing, and that it's doing only that.

I'm not fond of the serialize-to-rich-ID-and-parse-it-right-after for Term/Message/Attribute references.

I have to admit that I didn't look at the tests at all. Also something I defer to someone that wrote a resolver ;-)

spookylukey · 2019-01-29T13:03:13Z

I'd prefer the context manager and the sub-state to be explicit about "Variables in TermReferences". Make it non-generic? Took me a while to understand that that is what it's doing, and that it's doing only that.

I'm not fond of the serialize-to-rich-ID-and-parse-it-right-after for Term/Message/Attribute references.

Yep, one of the issues here was that I was backporting some code and some ways of doing things from later branches where they make more sense.

With regard to the first point you made, the 'current environment' concept does come in useful for other features, so it does seem more generic than for just term args. Perhaps it would help if there were comments on each element describing why if is needed? It seems like a waste to change it if I've got a good idea I'm going to change it back again later.

With the second point, I'll backport the rest of the changes which make the 'rich ID' more useful in several ways, which should clean up that ugly parsing of the rich ID you pointed out.

This has the result of performance optimization in a number of places, because we don't need to traverse attributes each time (with probably a very small startup cost), and simplifying quite a bit of logic. Also, tests for the corner cases found by looking at code coverage for the changes, and cleanups of places that were handling message IDs

spookylukey · 2019-01-29T18:31:49Z

@Pike - hopefully I've addressed most of the things you highlighted with my latest changes. There are more things in utils.py now, and it's not obvious yet why they don't live in resolver.py since that is the only place that uses them now, but they will be used by the compiler branch so I left them there.

In addition there is now b8aa39d which is both an optimization and a code cleanup.

stasm

This looks great, thanks @spookylukey. The code looks good to land, but I'd like to ask for two more test suites and I'd like to take a look at them before merging.

The first test suite should test that terms referenced within other terms always start with fresh args.

-foo = Foo is {$arg}
-bar = {-foo(arg: $arg)}
-baz = {-foo}
ref-bar = {-bar(arg: 1)}
ref-baz = {-bar(arg: 1)}

In the example above, ref-bar should format to 1 without erros while ref-baz should format to Foo is arg because arg hasn't been explicitly passed to -foo. This raises an interesting question about whether that should be an error or not. I think the current behavior of not reporting the error is OK to land, but I've take note of this and I'll try to come up with a solution in the spec.

In the second test suite, we should verify that messages referenced from within terms don't have access to external args.

msg = Msg is {$arg}
-foo = {msg}
ref-foo = {-foo(arg: 1)}

In this snippet, ref-foo should format to Msg is 1 even if format received external args of {'arg': 2}. This might be surprising, and perhaps we should forbid referencing messages from within terms in the future completely. For now however, this behavior adheres to the rule of creating fresh args for each TermReference and using it throughout its resolution.

stasm · 2019-01-30T14:07:07Z

fluent.runtime/setup.py

@@ -26,7 +26,7 @@
      ],
      packages=['fluent', 'fluent.runtime'],
      install_requires=[
-          'fluent>=0.9,<0.10',
+          'fluent.syntax>=0.10',


It might be a good idea to add an upper bound here as well. Syntax 0.9 will make a few changes to the AST (nothing extreme) which will likely break the resolver written against Syntax 0.8 (implemented by fluent.syntax 0.10).

stasm · 2019-01-30T14:11:48Z

fluent.runtime/fluent/runtime/utils.py

+    if isinstance(ref, AttributeExpression):
+        return _make_attr_id(reference_to_id(ref.ref),
+                             ref.name.name)
+    return (TERM_SIGIL if isinstance(ref, TermReference) else '') + ref.id.name


Nit (not blocking): Perhaps start with the if here, similar to how you check for AttributeExpression? And the same in ast_to_id. unknown_reference_error_obj already does this.

stasm · 2019-01-30T14:14:43Z

fluent.runtime/tox.ini

@@ -6,7 +6,7 @@ skipsdist=True
 setenv =
    PYTHONPATH = {toxinidir}
 deps =
-     fluent>=0.9,<0.10
+     fluent>=0.10


Should this read fluent.syntax? And again, it might be a good idea to specify the upper bound.

stasm · 2019-01-30T15:36:35Z

fluent.runtime/tests/format/test_parameterized_terms.py

+        # The '-thing' term should not get passed article="indefinite"
+        val, errs = self.ctx.format('thing-no-arg', {'article': 'indefinite'})
+        self.assertEqual(val, 'the thing')
+        self.assertEqual(errs, [])


Can you add a third variant to -thing for when no article is needed, [none] thing, and add a test similar to this one? Something like:

def test_no_implicit_access_to_external_args_but_term_args_still_passed(self): val, errs = self.ctx.format('thing-with-arg', {'article': 'other'}) self.assertEqual(val, 'a thing')

stasm · 2019-01-30T15:39:31Z

fluent.runtime/tests/format/test_parameterized_terms.py

+    def test_inner_arg(self):
+        val, errs = self.ctx.format('inner-arg', {})
+        self.assertEqual(val, 'The thing.')
+        self.assertEqual(errs, [])


Please add a test similar to TestParameterizedTerms.test_no_implicit_access_to_external_args which verifies that the args passed to the term replace external args fully, rather than be merged with them.

def test_inner_arg_with_external_args(self): val, errs = self.ctx.format('inner-arg', {'article': 'indefinite'}) self.assertEqual(val, 'The thing.')

stasm · 2019-01-30T16:26:13Z

I have two extra comments about the rich-id logic. They don't block this PR.

A consequence of the rich-id approach is that by storing the attributes separately from the messages they belong to, it forces any formatting API to be a pull API. What I mean by that is that the consumer of the library needs to know exactly the names of the attributes they request. I think this is fine for format, but at least in fluent.js we've seen use-cases for another API which we called compound (it hasn't landed yet). It takes a message id (no attribute paths allowed) and returns a {value, attributes} object. It's great for localizing DOM elements because there's only a single message lookup involved. The specifics of this API were the subject of a long discussion in projectfluent/fluent.js#208 (comment) and the outcome of the discussion was that there are good reasons for making the returned attributes both an iterator and a map allowing lookups, thus supporting both the pull and the push models. I'm saying all this to point out that the rich-id approach doesn't allow the push model , at least in its current form which breaks the O(1) mapping from messages to their attributes.

That said, I expect the rich-id logic to go away when we implement Syntax 0.9. If I understand correctly, it's currently used as way to simplify the handling of CallExpressions and AttributeExpressions where the parent is a TermReference. Without special handling, the TermReference would be first formatted to a string value by handle_term_reference, which would make it impossible to run the logic required by the CallExpression or the AttributeExpression on the Term AST node itself. (Cf. my comment about fully_resolve in #81 (review).) Syntax 0.9 (due in February) will remove CallExpressions and AttributeExpressions and instead store the attributes and call args on the reference node itself. See projectfluent/fluent#221 for more information about this and let me know if you have questions about this change.

Thanks again!

spookylukey · 2019-01-31T07:08:41Z

The first test suite should test that terms referenced within other terms always start with fresh args.
-foo = Foo is {$arg}
-bar = {-foo(arg: $arg)}
-baz = {-foo}
ref-bar = {-bar(arg: 1)}
ref-baz = {-bar(arg: 1)}
In the example above, ref-bar should format to 1 without erros while ref-baz should format to Foo is arg because arg hasn't been explicitly passed to -foo.

Is the definition of -bar here legal? With current master I get this:

>>> from fluent.syntax import parse
>>> parse("""-bar = { -foo(arg: $arg)}""").body
[<fluent.syntax.ast.Junk at 0x7f5f05ef76a0>]

This also appears to be following the spec correctly - named arguments can be only number or string literals:

stasm · 2019-01-31T08:08:00Z

Ah, my bad, you're right of course. That's projectfluent/fluent#230 (which I just filed because the original issue, 188, used outdated syntax and nomenclature). We'll consider extending the syntax to allow -foo(arg: $arg) after Fluent 1.0.

How about this test?

-foo = {$a} {$b}
-bar = {-foo(b: 2)}
-baz = {-foo}
ref-bar = {-bar(a: 1)}
ref-baz = {-baz(a: 1)}

ref-bar → a 2
ref-baz → a b

@stasm

Thanks @stasm

spookylukey · 2019-01-31T17:24:37Z

I think that's address everything you mentioned now.

stasm

Thanks, LGTM.

Updated fluent.runtime to fluent.syntax 0.10 and implemented new feat…

82f985f

…ures.

Pike reviewed Jan 28, 2019

View reviewed changes

Pike requested a review from stasm January 28, 2019 13:08

spookylukey added 3 commits January 29, 2019 18:41

Cleaned up some return/else control flow to be more consistent

628ea31

resolver: code comments for CurrentEnvironment

59292d6

spookylukey force-pushed the update_fluent_runtime_for_fluent_syntax_010 branch from bc1eaf6 to 59292d6 Compare January 29, 2019 18:31

stasm suggested changes Jan 30, 2019

View reviewed changes

spookylukey added 3 commits January 31, 2019 20:19

format - some additional tests for messages/terms in terms

9914930

Thanks @stasm

More consistent if/else pattern in ast_to_id, reference_to_id

1cbb488

fluent.runtime - upper bound on fluent.syntax dependency

30eb856

stasm approved these changes Feb 1, 2019

View reviewed changes

spookylukey merged commit 9c1b457 into projectfluent:master Feb 1, 2019

spookylukey deleted the update_fluent_runtime_for_fluent_syntax_010 branch March 3, 2019 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update fluent.runtime for fluent.syntax 0.10 #88

Update fluent.runtime for fluent.syntax 0.10 #88

spookylukey commented Jan 28, 2019

Pike left a comment

spookylukey commented Jan 29, 2019

spookylukey commented Jan 29, 2019 •

edited

Loading

stasm left a comment

stasm Jan 30, 2019

stasm Jan 30, 2019

stasm Jan 30, 2019

stasm Jan 30, 2019

stasm Jan 30, 2019

stasm commented Jan 30, 2019

spookylukey commented Jan 31, 2019

stasm commented Jan 31, 2019

spookylukey commented Jan 31, 2019

stasm left a comment

Update fluent.runtime for fluent.syntax 0.10 #88

Update fluent.runtime for fluent.syntax 0.10 #88

Conversation

spookylukey commented Jan 28, 2019

Pike left a comment

Choose a reason for hiding this comment

spookylukey commented Jan 29, 2019

spookylukey commented Jan 29, 2019 • edited Loading

stasm left a comment

Choose a reason for hiding this comment

stasm Jan 30, 2019

Choose a reason for hiding this comment

stasm Jan 30, 2019

Choose a reason for hiding this comment

stasm Jan 30, 2019

Choose a reason for hiding this comment

stasm Jan 30, 2019

Choose a reason for hiding this comment

stasm Jan 30, 2019

Choose a reason for hiding this comment

stasm commented Jan 30, 2019

spookylukey commented Jan 31, 2019

stasm commented Jan 31, 2019

spookylukey commented Jan 31, 2019

stasm left a comment

Choose a reason for hiding this comment

spookylukey commented Jan 29, 2019 •

edited

Loading