Indentation/Whitespace in 0.7 #284

zbraniecki · 2018-09-14T06:32:55Z

This PR supersets #272. It covers all of the 0.7 indentation changes I could find, aligning the parser with the grammar.mjs as close I dared.

I think all the resulting test changes make sense as well. :)

Pike

I've given this a glance, and I realize that I'm not as good a reviewer as stas hoped.

Big picture, can you rebase and target the zeroseven branch for this?

I've tried my best to go through the test changes, and the fixtures_structure ones I agree with.

I have some open questions on the behavior ones, and I didn't go through the whole test suite to see if there are more changes that I'd expect. There is at least one in indent.ftl, though.

Pike · 2018-09-14T13:07:05Z

fluent-syntax/src/ftlstream.js

@@ -5,10 +5,11 @@ import { ParseError } from "./errors";
 import { includes } from "./util";

 const INLINE_WS = [" ", "\t"];
+const ANY_WS = [" ", "\t", "\r", "\n"];


Not sure if you want to do this as part of this patch or not, but \t is a regular char now.

oh, cool! updated!

Pike · 2018-09-14T13:25:12Z

fluent-syntax/test/fixtures_behavior/placeable_in_placeable.ftl

@@ -8,7 +8,7 @@ key2 = {  { foo }  }
 #   }

 key4 = {  { foo }
-# ~ERROR E0003, pos 93, args "}"
+# ~ERROR E0003, pos 96, args "}"


This is weird. Why is # OK in a placeable?

It's not. The error is exactly on that character:

! E0003 on line 2: | key4 = { { foo } | # ~ERROR E0003, pos 93, args "}" … ^----- Expected token: "}"

Ok, so we dove into this with Axel. The reason this annotation stops at 96 now, is that the test infra removes the # ~ERROR comment, so the parser continues and stops on the first character of # key5 two \n's later which is 96 :)

Pike · 2018-09-14T13:27:25Z

fluent-syntax/test/fixtures_behavior/indent.ftl

@@ -8,7 +8,6 @@ a }

 key3 = { a
 }
-# ~ERROR E0003, pos 36, args "}"

 key4 = {
 { a }}


The key4 error should also go away, I think.

zbraniecki · 2018-09-14T18:02:30Z

Updated to your feedback! Thank you! :)

stasm

Thanks, @zbraniecki, for this PR! I think this is pretty close. The one big thing which I see here is that this PR doesn't support unindented placeables yet. I would also like to see all other reference fixtures copied into fluent-syntax/test/fixtures_reference to make sure we're not missing anything else.

stasm · 2018-09-20T12:31:13Z

fluent-syntax/src/ftlstream.js


-    if (this.currentPeekIs("*")) {
+    if (def) {
+      this.skipToPeek();
      this.peek();
    }

    if (this.currentPeekIs("[") && !this.peekCharIs("[")) {


Drive-by comment: is !this.peekCharIs("[") a remnant of sections?

stasm · 2018-09-20T12:31:55Z

fluent-syntax/src/ftlstream.js

@@ -4,11 +4,12 @@ import { ParserStream } from "./stream";
 import { ParseError } from "./errors";
 import { includes } from "./util";

-const INLINE_WS = [" ", "\t"];
+const INLINE_WS = [" "];


Since space is the only valid inline blank character, maybe just define it as:

const INLINE_WS = " "

?

stasm · 2018-09-20T12:53:34Z

fluent-syntax/src/parser.js

-      ps.peekInlineWS();
-      if (ps.isPeekNextLineVariantStart()) {
+      ps.peekBlank();
+      if (ps.currentPeek() === "*" || ps.currentPeek() === "[") {


I think I'd prefer to stick to the existing abstraction in form of isPeekNextLineVariantStart here, even if it means calling peekBlankInline first.

So, this is a bit more tricky. The situation is unusual in this place because we want to try to get a variant list, but if that doesn't work, we bail back to the { and go for getPattern.

That means that if we transition to use isPeekNextLineVariantStart I need to complicate it to not always skip.

stasm · 2018-09-20T13:09:57Z

fluent-syntax/src/ftlstream.js

      return true;
    }

    this.resetPeek();
    return false;
  }

-  isPeekNextLineValue() {
+  isPeekNextLineValue(skipToPeek = true) {


Given that all isPeek... methods skip by default now, maybe we should call them simply isNextLineValue() etc?

Perhaps it's just me, but I often forget which methods skip and which do not. Making it a bit explicit would certainly make the code easier to follow for me:

isPeekNextLineValue({skip}) { ... }

Then, we could use it everywhere in a self-documenting fashion:

ps.isNextLineValue({skip: true}) ps.isNextLineValue({skip: false})

For consistency, I'd make this change to all isSomething methods.

The previous in turn means that now not all of them skip, most of them do, but one of them doesn't, which makes it inconsistent as well :(

Sorry, I didn't understand your comment. Do you agree that making the skipping behavior explicit in the parameter passed into all isSomething methods helps understand the reader what the method will do? Like this:

ps.isPeekNextLineValue({skip: true}) ps.isPeekNextLineValue({skip: false})

I'd get rid of Peek in the names of these methods to make them shorter. Which gives:

ps.isNextLineValue({skip: true}) ps.isNextLineValue({skip: false})

stasm · 2018-09-20T13:17:44Z

fluent-syntax/src/parser.js

    }

    return this.getPattern(ps);
  }

  getVariantList(ps) {
    ps.expectChar("{");
-    ps.skipInlineWS();
+    ps.skipBlank();


I think this needs to ensure at least one line break before moving on to parsing variants. Same as you do in getExpression.

stasm · 2018-09-20T13:39:59Z

fluent-syntax/src/parser.js


    let ch;
    while ((ch = ps.current())) {

      // The end condition for getPattern's while loop is a newline
      // which is not followed by a valid pattern continuation.
-      if (ch === "\n" && !ps.isPeekNextLineValue()) {
+      if (ch === "\n" && !ps.isPeekNextLineValue(false)) {
        break;


We shouldn't break here before first checking if there's an unindented placeable on the next line. The following snippets are valid Fluent 0.7:

key1 = { placeable} key2 = { $selector -> *[one] One }

The reference parser has a number of fixtures for testing this in select_indent.ftl and variants_indent.

This might require a different logic than the one provided by isPeekNextLineValue. If so, my other comment about parametrizing {skip: true} might not hold anymore. I'd still rename isPeekSomething to isSomething, though.

zbraniecki · 2018-09-20T23:35:01Z

Updated to review, and copied reference tests. :) I was even able to remove a couple from the blacklist!

stasm

This looks great, thanks for a quick turn-around! I'd like to get a clarification about your answer to my {skip: true} suggestion before approving this PR. Otherwise, this is ready to land.

stasm · 2018-09-21T11:59:47Z

fluent-syntax/src/ftlstream.js

      return true;
    }

    this.resetPeek();
    return false;
  }

-  isPeekNextLineValue() {
+  isPeekNextLineValue(skipToPeek = true) {


Sorry, I didn't understand your comment. Do you agree that making the skipping behavior explicit in the parameter passed into all isSomething methods helps understand the reader what the method will do? Like this:

ps.isPeekNextLineValue({skip: true}) ps.isPeekNextLineValue({skip: false})

I'd get rid of Peek in the names of these methods to make them shorter. Which gives:

ps.isNextLineValue({skip: true}) ps.isNextLineValue({skip: false})

stasm

Yay, thanks, @zbraniecki!

* Indentation/Whitespace in 0.7 * Apply feedback * Apply 2nd round of feedback

zbraniecki requested review from Pike and stasm September 14, 2018 06:33

Pike reviewed Sep 14, 2018

View reviewed changes

zbraniecki force-pushed the 0.7-indent branch from 1eaae0a to 3755c5b Compare September 14, 2018 17:49

zbraniecki changed the base branch from master to zeroseven September 14, 2018 17:49

zbraniecki force-pushed the 0.7-indent branch 2 times, most recently from 19508ea to c9e3c8d Compare September 14, 2018 17:56

Indentation/Whitespace in 0.7

9f1858c

zbraniecki force-pushed the 0.7-indent branch from c9e3c8d to 9f1858c Compare September 14, 2018 17:58

stasm suggested changes Sep 20, 2018

View reviewed changes

Apply feedback

ccdf38e

zbraniecki requested a review from stasm September 20, 2018 23:31

stasm reviewed Sep 21, 2018

View reviewed changes

Apply 2nd round of feedback

54d1dde

zbraniecki requested a review from stasm September 21, 2018 16:21

stasm approved these changes Sep 24, 2018

View reviewed changes

zbraniecki merged commit 84208ce into projectfluent:zeroseven Sep 24, 2018

zbraniecki mentioned this pull request Sep 24, 2018

Relax indentation WIP #272

Closed

stasm pushed a commit that referenced this pull request Oct 12, 2018

Indentation/Whitespace in 0.7 (#284)

52e1854

* Indentation/Whitespace in 0.7 * Apply feedback * Apply 2nd round of feedback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indentation/Whitespace in 0.7 #284

Indentation/Whitespace in 0.7 #284

zbraniecki commented Sep 14, 2018

Pike left a comment

Pike Sep 14, 2018

zbraniecki Sep 14, 2018

Pike Sep 14, 2018

zbraniecki Sep 14, 2018

zbraniecki Sep 14, 2018

Pike Sep 14, 2018

zbraniecki commented Sep 14, 2018

stasm left a comment

stasm Sep 20, 2018

stasm Sep 20, 2018

stasm Sep 20, 2018

zbraniecki Sep 20, 2018

stasm Sep 20, 2018

stasm Sep 20, 2018

zbraniecki Sep 20, 2018

stasm Sep 21, 2018

stasm Sep 20, 2018

stasm Sep 20, 2018

stasm Sep 20, 2018

zbraniecki commented Sep 20, 2018

stasm left a comment

stasm Sep 21, 2018

stasm left a comment

Indentation/Whitespace in 0.7 #284

Indentation/Whitespace in 0.7 #284

Conversation

zbraniecki commented Sep 14, 2018

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

zbraniecki commented Sep 14, 2018

stasm 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

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 commented Sep 20, 2018

stasm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm left a comment

Choose a reason for hiding this comment