Introduce private messages. #68

stasm · 2017-11-14T13:38:11Z

Fix #62.

Private messages are similar to regular messages but they can only be
used as references in other messages. Their identifiers start with at
least one dash -, e.g. -brand-name. The runtime cannot retrieve
private messages directly. They are best used to define glossary terms
which can be used consistently across the localization of the entire
product.

Private messages can store some grammatical information about themselves
in tags. Private messages are not allowed to have attributes.

stasm · 2017-11-14T16:16:01Z

spec/fluent.asdl

          | Section(symb name, comment? comment)
          | Comment(comment)
          | Junk(str content)
          attributes (annot* annotations, span? span)

+    base_message = Message(pat? value, attr* attributes)
+                 | PrivateMessage(pat value, tag* tags)


This makes it a requirement that PrivateMessages have a non-null value. Should we enforce this for now or keep it consistent with public Messages?

I'm ok enforcing it.

zbraniecki · 2017-11-14T18:58:59Z

spec/fluent.ebnf

-identifier           ::= [a-zA-Z] [a-zA-Z0-9_-]*
+public-identifier    ::= [a-zA-Z] [a-zA-Z0-9_-]*
+private-identifier   ::= '-'+ public-identifier
+identifier           ::= public-identifier | private-identifier
 external             ::= '$' identifier


I think you need to change external to only accept public-identifier. Or, maybe I'd even change public-identifier to identifier, and then create message-identifier ::= identifier | private-identifier.

Good call, I like this approach better. Thanks.

zbraniecki · 2017-11-14T18:59:18Z

spec/fluent.ebnf

@@ -41,7 +43,9 @@ tag-list             ::= NL (__ tag)+
 attribute            ::= NL __ '.' identifier value


Pike

The asdl changes are a bit above my head, mostly because I don't know if there's a difference between a param thingie and an attributes thingie.

I'd really like ensure that we don't expose attributes on private messages, probably already in this patch.

For the general patch queue, the change log might end up being confusing by adding private messages with some properties, and then removing them again in the next patch. Is that a concern?

Pike · 2017-11-29T13:52:18Z

guide/external.md

+and are called placeables.
+
+It's common to used placeables to interpolate an external argument, provided
+by the developer, into the string.


to used -> to use, also, I'd put the provided by the developer to the end, the sentence is really broken up right now.

It's common to use placeables to interpolate an external argument into the string, provided by the developer doesn't sound right either. I'll go with It's common to use placeables to interpolate external arguments into the translation. External arguments are provided by the developer and may change on runtime.

Pike · 2017-11-29T14:22:15Z

spec/fluent.ebnf

@@ -69,8 +73,8 @@ block-expression     ::= select-expression
                       | variant-list

 select-expression    ::= inline-expression __ '->' __ variant-list
-attribute-expression ::= identifier '.' identifier
-variant-expression   ::= identifier '[' _? variant-key _? ']'
+attribute-expression ::= message-identifier '.' identifier


If this is just identifier, the syntax defines that we can't dereference attributes of private messages?

That'd make inline-expression safe, and we could use an unsafe-inline-expression that adds private-indentifier.identifier as selector-expression instead of inline-expression?

Nice, I like this approach. It makes syntax work for us without adding more... syntax.

We could even go further and define selector-expression which would be different from inline-expression and block-expression and could comprise of:

inline-expression ::= quoted-text | number | identifier | external | variant-expression | public-attribute-expression | call-expression | placeable block-expression ::= select-expression | variant-list selector-expression ::= quoted-text | number | identifier | external | public-attribute-expression | private-attribute-expression | call-expression

Compare it with:

fluent/spec/fluent.ebnf

Line 60 in b9552ae

inline-expression ::= quoted-text

Yeah, agreed, allowing placeable or variant-expression doesn't make much sense in a selector-expression

stasm · 2017-11-30T10:54:20Z

The asdl changes are a bit above my head, mostly because I don't know if there's a difference between a param thingie and an attributes thingie.

attributes can be used to avoid repetition of fields in unions. The following two notations are equivalent:

prod = Foo(string name)
     | Bar(string name)

prod = Foo
     | Bar
     attributes (string name)

The relevant excerpt from The Zephyr Abstract Syntax Description Language, p. 5, § 2.6:

Often several constructors of a type share a set of common values. To make this explicit, ASDL includes an attribute notation. Since all variants of a type carry the values of an attribute, its fields can be accessed without having to discriminate between the various constructors. Attributes can be seen as providing some limited features of inheritance

Pike

I wonder if we should open a ticket to allow selectors based on translated text. We had numerous requests for that in the past. Or do we only want to allow that in GLOBALS?

Pike · 2017-11-30T13:21:55Z

guide/private.md

+Private messages are similar to regular messages but they can only be used as
+references in other messages. Their identifiers start with at least one dash
+`-` like in the example above: `-brand-name`. The runtime cannot retrieve
+private messages directly. They are best used to define glossary terms which


Just dropping the glossary here alltogether sounds good enough for me.

Just to make sure I understand this comment, you'd rather see …to define terms which…?

Pike · 2017-11-30T13:27:03Z

spec/fluent.asdl

@@ -40,19 +44,16 @@ module Fluent
         | Placeable(expr expression, span? span)

    -- Expressions
-    expr = InlineExpression(inline_expr)
-         | BlockExpression(block_expr)
-    inline_expr = StringExpression(string)


inline_expr is still used in line 70?

arg = Expression(inline_expr)

Pike · 2017-11-30T13:40:03Z

spec/fluent.ebnf

+selector-expression  ::= quoted-text
+                       | number
+                       | external-identifier
+                       | public-identifier


This should be private-identifier instead of public-identifier, as tags are only on private messages, and I guess the intent is to not select based on translation value?

Good catch and yes, that's the intent. At least for now.

Fix projectfluent#62. Private messages are similar to regular messages but they can only be used as references in other messages. Their identifiers start with at least one dash `-`, e.g. `-brand-name`. The runtime cannot retrieve private messages directly. They are best used to define glossary terms which can be used consistently across the localization of the entire product. Private messages can store some grammatical information about themselves in tags. Private messages are not allowed to have attributes.

The new `selector-expression` EBNF symbol restricts which expressions can be selectors. The following expressions are now illegal as selectors: - references to public messages, - attribute expressions on public messages, - variant expressions, - select expressions (even when wrapped in a placeable).

stasm requested review from Pike and zbraniecki November 14, 2017 13:38

stasm commented Nov 14, 2017

View reviewed changes

zbraniecki requested changes Nov 14, 2017

View reviewed changes

stasm force-pushed the private-messages branch from 9670021 to 675aae2 Compare November 15, 2017 11:00

stasm requested a review from zbraniecki November 15, 2017 11:01

stasm force-pushed the private-messages branch from 675aae2 to 4018791 Compare November 15, 2017 11:16

This was referenced Nov 15, 2017

Replace tags with attributes defined on private messages. #69

Merged

Remove Sections, introduce group and file comments #70

Merged

zbraniecki approved these changes Nov 16, 2017

View reviewed changes

stasm mentioned this pull request Nov 23, 2017

Introduce Glossary Messages #62

Closed

3 tasks

Pike reviewed Nov 29, 2017

View reviewed changes

stasm force-pushed the private-messages branch from 4018791 to bf90ef2 Compare November 30, 2017 12:17

stasm requested a review from Pike November 30, 2017 12:24

stasm force-pushed the private-messages branch 2 times, most recently from 1f4b490 to 7619b5c Compare November 30, 2017 12:36

Pike suggested changes Nov 30, 2017

View reviewed changes

stasm added 2 commits November 30, 2017 14:54

stasm force-pushed the private-messages branch from 7619b5c to b690637 Compare November 30, 2017 13:55

stasm requested a review from Pike November 30, 2017 13:56

Pike approved these changes Nov 30, 2017

View reviewed changes

stasm merged commit af600be into projectfluent:master Nov 30, 2017

stasm deleted the private-messages branch November 30, 2017 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce private messages. #68

Introduce private messages. #68

stasm commented Nov 14, 2017

stasm Nov 14, 2017

zbraniecki Nov 14, 2017

zbraniecki Nov 14, 2017

stasm Nov 15, 2017

zbraniecki Nov 14, 2017

Pike left a comment

Pike Nov 29, 2017

stasm Nov 30, 2017

Pike Nov 29, 2017

stasm Nov 29, 2017 •

edited

Loading

Pike Nov 29, 2017

stasm commented Nov 30, 2017 •

edited

Loading

Pike left a comment

Pike Nov 30, 2017

stasm Nov 30, 2017

Pike Nov 30, 2017

Pike Nov 30, 2017

stasm Nov 30, 2017

Pike Nov 30, 2017

stasm Nov 30, 2017

		@@ -41,7 +43,9 @@ tag-list ::= NL (__ tag)+
		attribute ::= NL __ '.' identifier value

Introduce private messages. #68

Introduce private messages. #68

Conversation

stasm commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm commented Nov 30, 2017 • edited Loading

Pike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Nov 29, 2017 •

edited

Loading

stasm commented Nov 30, 2017 •

edited

Loading