Remove Sections, introduce group and file comments #70

stasm · 2017-11-15T13:58:57Z

Fixes #58. Requires #68 and #69 (both of which are included in this PR right now). Please only review the last commit.

stasm · 2017-11-15T14:10:35Z

spec/fluent.asdl

    fun = Function(str name, span? span)
    string = StringExpression(str value, span? span)
    number = NumberExpression(str value, span? span)

-    comment = Comment(str content, span? span)
+    comment = Comment(int level, str content, span? span)


Would it better to introduce three node types instead of the level field? E.g. SingleComment, DoubleComment and TripleComment.

Not sure which way to go. The concern I have is about semantically correct mixed level comments:

# Foo ### Bar # Baz ## Qux key = Value

These would be four independent Comment entries with levels: 1, 3, 1, and 2. Or four independent entries: SingleComment, TripleComment, SingleComment, and a DoubleComment.

Would it better to introduce three node types instead of the level field? E.g. SingleComment, DoubleComment and TripleComment.

@Pike, do you have an opinion or a recommendation about this?

I wouldn't call 'em by markup, but by purpose. So, FileComment, GroupComment, MessageComment or just Comment. Or just leave them with levels.

I went for Comment, GroupComment and FileComment. They are explicit and clearly communicate the role. They also define the interpretation of each of the comment types right at the level of the syntax which will will improve the portability of FTL files.

zbraniecki

I struggle a bit to understand how this PR changes the relative positioning of comments and how it impacts their role.
Before this PR it's very clear to me that a comment at the very top of the file is "attached" to it and thus is a "File Level Comment". A comment attached to a message is a "Message Level Comment" and so on.

With this changes I feel like we're getting into a world where there may be multiple "File Level Comments" spread around the file and the distinction between a comment attached to a Message and "standalone" is also unclear to me.
What should happen if the comment attached to a Message is a Group Level Comment? Or File Level Comment?

Lastly, I'm wondering how to translate the new group level comments into a visual representation in a GUI like Pontoon.

For example, with the following FTL:

[[ Menu Items ]]
// Here is a set of menu items
// which you can attach to a menu

key1 = Value
key2 = Value 2

I imagined that in the entry list panel in Pontoon a user would only see the title of the section and maybe an outline around the group of messages affected, and then the comment for that section would be displayed by each of the messages in the main pane.

With the new system, I don't see a good way except of some enforced best-practice to make sure that the first line of the group level comment actually is a "title" of such group and the following lines are describing the group.

Is that not a concern for you?

zbraniecki · 2017-11-20T13:49:30Z

spec/fluent.asdl

@@ -23,10 +23,9 @@

 module Fluent
 {
-    res = Resource(entry* body, comment? comment, span? span)
+    res = Resource(entry* body, span? span)


What is the expected outcome of "file-level" comment being placed in the middle of the file? Say:

key = Message # Message level comment key2 = Message 2 ### Suddenly, a wild file-level message appears key3 = Message 3

It appears in the AST a such: a Comment entry between key2 and key3.

zbraniecki · 2017-11-20T13:50:43Z

spec/fluent.asdl

    fun = Function(str name, span? span)
    string = StringExpression(str value, span? span)
    number = NumberExpression(str value, span? span)

-    comment = Comment(str content, span? span)
+    comment = Comment(int level, str content, span? span)


Not sure which way to go. The concern I have is about semantically correct mixed level comments:

# Foo ### Bar # Baz ## Qux key = Value

stasm · 2017-11-20T14:15:06Z

Before this PR it's very clear to me that a comment at the very top of the file is "attached" to it and thus is a "File Level Comment".

FWIW, this has never been as clear to me. I find it confusing that the the file-level comment is the first inside of the file while the message-level comment is the first outside (and before) the message.

A comment attached to a message is a "Message Level Comment" and so on.

This doesn't change for # comments. Only # comments can be attached to a message.

With this changes I feel like we're getting into a world where there may be multiple "File Level Comments" spread around the file (…)

Yes, that's now possible, but I expect the linter to warn against this. In most cases we would only care about the first file-level comment.

the distinction between a comment attached to a Message and "standalone" is also unclear to me.
What should happen if the comment attached to a Message is a Group Level Comment? Or File Level Comment?

Only # comments can be attached to a message. Thus the following:

## Group Level Comment
foo = Foo

…parses as two separate entries: a Comment and a Message.

stasm · 2017-11-20T14:18:42Z

With the new system, I don't see a good way except of some enforced best-practice to make sure that the first line of the group level comment actually is a "title" of such group and the following lines are describing the group.

With this change I'm betting on the fact the developers don't care about the titles; they care about the comment. For example (source):

// Page titles, put in the <title> HTML tag.
[[pageTitle]]
pageTitleDefault = Firefox Test Pilot
pageTitleLandingPage = Firefox Test Pilot
pageTitleExperimentListPage = Firefox Test Pilot - Experiments
pageTitleExperiment = Firefox Test Pilot - {$title}

i think Pontoon should show the entire Group Comment for each message.

Pike · 2017-11-29T14:34:45Z

spec/CHANGELOG.md

+    `Resource` node has been removed.
+
+  - Renamed `Symbol` to `VariantName` in the ASDL.
+


Should this add something about backwards compatiblity? Is this something we do on the fluent side or on the implementations side?

If it's impl, should there be something here that suggests that impls do this?

What do you mean by add something?

Should we document how we expect implementations to deal with the update from syntax 0.4 to 0.5?

I think we should suggest a way forward when we release 0.5. It will be based on the approach we'll take with our implementations.

Pike · 2017-11-29T14:39:02Z

spec/fluent.ebnf

@@ -31,8 +32,7 @@ word                 ::= (((char - line-break) - inline-space) - [#x5b#x5c#x5d#x
 builtin              ::= [A-Z] [A-Z_?-]*
 number               ::= '-'? [0-9]+ ('.' [0-9]+)?

-variant-key          ::= number | variant-symbol
-variant-symbol       ::= word (_ word)*
+variant-key          ::= word (_ word)*


Is this change intentional?

Yes. The word production already allows all characters that number did.

Hmm, but having two numbers next to each other separated by a space is not what we want. OK, I'll bring this back.

zbraniecki

yay!

zbraniecki · 2017-12-12T03:49:14Z

guide/comments.md

+### Localization for Server-side strings of Firefox Screenshots
+### Please don't localize Firefox, Firefox Screenshots, or Screenshots
+
+## Global phrases shared across pages


## Global phrases ## ## Phrases shared accross pages.

Not sure that's readable in tools like pontoon. I think we should advertise single-line or at least format-flowed comments.

If I understand @zbraniecki's intent correctly, Pontoon could display only the first line of the comment in places where screen space is tight. This would be similar to how Git treats commit messages.

This is a good practice that we should discuss separately. For now, I made all comments in the PR single-line.

zbraniecki · 2017-12-12T04:00:15Z

spec/fluent.ebnf

-section              ::= '[[' _? word (_ word)* _? ']]'
+comment              ::= ('# ' (char - NL)* NL)+
+                       | ('## ' (char - NL)* NL)+
+                       | ('### ' (char - NL)* NL)+


switch to inline-space instead of ` space char following the sygil

Pike · 2017-12-13T22:24:21Z

guide/comments.md

+### Localization for Server-side strings of Firefox Screenshots
+### Please don't localize Firefox, Firefox Screenshots, or Screenshots
+
+## Global phrases shared across pages


Not sure that's readable in tools like pontoon. I think we should advertise single-line or at least format-flowed comments.

Pike · 2017-12-13T22:28:37Z

guide/comments.md

+files into smaller groups of messages related to each other; they are
+group-level comments. Think of them as of headers with a description.
+Group-level comments are intended as a hint for localizers and tools about
+the layout of the localization resource.


Can you add docs on how to end a group? Aka, something like

The grouping ends with the next group comment. Use an empty group comment if there's no following group.

Or something not that butchered.

stasm requested review from Pike and zbraniecki November 15, 2017 14:01

stasm commented Nov 15, 2017

View reviewed changes

zbraniecki reviewed Nov 20, 2017

View reviewed changes

stasm mentioned this pull request Nov 23, 2017

Comment Levels #58

Closed

3 tasks

Pike reviewed Nov 29, 2017

View reviewed changes

stasm force-pushed the comment-levels branch 3 times, most recently from f54251d to 52aa384 Compare December 6, 2017 20:03

stasm changed the title ~~Comment levels~~ Remove Sections, introduce group and file comments Dec 6, 2017

stasm requested review from Pike and zbraniecki December 6, 2017 20:10

zbraniecki approved these changes Dec 12, 2017

View reviewed changes

stasm force-pushed the comment-levels branch from 52aa384 to 812a94c Compare December 13, 2017 17:17

Pike approved these changes Dec 13, 2017

View reviewed changes

Remove sections; introduce Comment, GroupComment, ResourceComment

1e3ff3e

stasm force-pushed the comment-levels branch from 9d86427 to 1e3ff3e Compare December 20, 2017 11:55

stasm merged commit 25c892c into projectfluent:master Dec 20, 2017

stasm deleted the comment-levels branch December 20, 2017 12:04

stasm mentioned this pull request Jan 5, 2018

Multiline comments projectfluent/fluent.js#121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Sections, introduce group and file comments #70

Remove Sections, introduce group and file comments #70

stasm commented Nov 15, 2017

stasm Nov 15, 2017

zbraniecki Nov 20, 2017

stasm Nov 20, 2017

stasm Dec 1, 2017

Pike Dec 1, 2017

stasm Dec 6, 2017

zbraniecki left a comment

zbraniecki Nov 20, 2017

stasm Nov 20, 2017

zbraniecki Nov 20, 2017

stasm commented Nov 20, 2017

stasm commented Nov 20, 2017

Pike Nov 29, 2017

stasm Dec 1, 2017

Pike Dec 1, 2017

stasm Dec 1, 2017

Pike Nov 29, 2017

stasm Nov 30, 2017

stasm Nov 30, 2017

zbraniecki left a comment

zbraniecki Dec 12, 2017

Pike Dec 13, 2017

stasm Dec 20, 2017

zbraniecki Dec 12, 2017

Pike Dec 13, 2017

Pike Dec 13, 2017

		`Resource` node has been removed.

		- Renamed `Symbol` to `VariantName` in the ASDL.

Remove Sections, introduce group and file comments #70

Remove Sections, introduce group and file comments #70

Conversation

stasm commented Nov 15, 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

Choose a reason for hiding this comment

zbraniecki 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 commented Nov 20, 2017

stasm commented Nov 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki 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