Add AST.Placeable #64
Conversation
stasm
force-pushed
the
ast-placeable
branch
5 times, most recently
from
1f43bf5 to
99b84b7
Compare
| break; | ||
| } | ||
|
|
||
| if (ch === '{') { |
There was a problem hiding this comment.
Should we make the closing brace } special as well? It's not special right now, i.e. it doesn't need to be escaped to be part of a TextElement if it's not matched. This makes the following FTL valid: key = { foo } }, with <space>} being a TextElement.
There was a problem hiding this comment.
I'd prefer to not enforce escaping if it's not necessary
There was a problem hiding this comment.
Well, my question is then: do you think it's necessary? No syntax error in key = { foo } } is a bit weird to me.
There was a problem hiding this comment.
sorry, I should have state my opinion. I don't believe that it's necessary. I think it only looks weird because of the semantic meaning you give to } when seen in this context.
I'd argue that:
foo = Bar = Baz
looks equally weird. :)
There was a problem hiding this comment.
OK, that's a good point, thanks!
| @@ -0,0 +1,3 @@ | |||
| export function includes(arr, elem) { | |||
| return arr.indexOf(elem) > -1; | |||
There was a problem hiding this comment.
String.prototype.includes is quite well-supported but since this quasi-polyfill is so small and simple I thought there was not point in artificially excluding IE11. Note that our compat builds don't polyfill includes automatically.
There was a problem hiding this comment.
could we get a compat-utils that are included only in the compat build? :) I think it would be nice to invest a bit of our time to design a build system for fluent which does not penalize evergreens for the sins of their ancestors. Although I agree that in this case the polyfill is small :)
There was a problem hiding this comment.
We could change String.prototype and Array.prototype and somehow do this only for the compat build. I don't think it's worth it though. Almost all of our needs are covered by the current pipeline. includes is a rare exception.
There was a problem hiding this comment.
Nah, let's do what you did in your patch.
|
|
||
| return new AST.SelectExpression(null, variants); | ||
| } | ||
|
|
||
| ps.skipInlineWS(); |
There was a problem hiding this comment.
I think this should be skipWS really to allow newlines after the opening { but I couldn't quite track down a few test failures. I went for skipInlineWS for now to stay close to the current implementation. We'll need a whole new PR to allow newlines in different places anyways.
There was a problem hiding this comment.
I filed #67 to track this.
| @@ -1,3 +1,2 @@ | |||
| key = Value | |||
| Value 2 | |||
| //~ ERROR E0002, pos 12 | |||
There was a problem hiding this comment.
This is allowed by the EBNF and should not be a syntax error. (It might be a linting error however.)
| @@ -84,22 +87,6 @@ export class FTLParserStream extends ParserStream { | |||
| return ((cc >= 48 && cc <= 57) || cc === 45); // 0-9 | |||
| } | |||
|
|
|||
| isPeekNextLineIndented() { | |||
There was a problem hiding this comment.
This wasn't used anywhere.
| @@ -208,7 +195,7 @@ export class FTLParserStream extends ParserStream { | |||
| } | |||
|
|
|||
| skipToNextEntryStart() { | |||
| while (this.next()) { | |||
| while (this.ch) { | |||
There was a problem hiding this comment.
When the syntax error happens exactly on the newline character, this.next() would overshoot the beginning of the entry on the next line, if present.
| // Find the last line break starting backwards from the index just before | ||
| // pos. This allows us to correctly handle ths case where the character at | ||
| // pos is a line break as well. | ||
| const fromIndex = pos - 1; |
There was a problem hiding this comment.
Without this, columnOffset was returning 0 when the character at pos was a new line.
| return new AST.Pattern(elements); | ||
| getPlaceable(ps) { | ||
| ps.expectChar('{'); | ||
| const expression = this.getExpression(ps); |
There was a problem hiding this comment.
getExpression should know how to handle nested placeables. I filed #66 for this.
|
|
||
| switch (expr.type) { | ||
| case 'Placeable': | ||
| return `{${serializePlaceable(expr)}}`; |
There was a problem hiding this comment.
I can't test this yet because the parser doesn't support nested Placeables.
| // Special-case: trim leading whitespace and newlines. | ||
| if (ps.isPeekNextLinePattern()) { | ||
| ps.skipBlankLines(); | ||
| ps.skipInlineWS(); |
There was a problem hiding this comment.
it would be nice if we could just peek to the first character that is non-WS and if it's a pattern start, skip to peek here, instead of resetting and skipping again, but since it's not a runtime parser, I don't think it's a blocker.
There was a problem hiding this comment.
Yeah, I kept this to stay in line with other isPeek... methods which always reset the peek. We could try changing them such that they skipToPeek before returning true, although that would still need a bit of hand-holding in isPeekNextLineVariantStart.
- Implement Fluent Syntax 0.5.
- Add support for terms.
- Add support for `#`, `##` and `###` comments.
- Remove support for tags.
- Add support for `=` after the identifier in message and term
defintions.
- Forbid newlines in string expressions.
- Allow trailing comma in call expression argument lists.
In fluent-syntax 0.6.x the new Syntax 0.5 is supported alongside the old
Syntax 0.4. This should make migrations easier.
`FluentParser` will correctly parse Syntax 0.4 comments (prefixed with
`//`), sections and message definitions without the `=` after the
identifier. The one exception are tags which are no longer supported.
Please use attributed defined on terms instead.
`FluentSerializer` always serializes using the new Syntax 0.5.
- Add `AST.Placeable` (#64)
Added in Syntax Spec 0.4, `AST.Placeable` provides exact span data about
the opening and closing brace of placeables.
- Expose `FluentSerializer.serializeExpression`. (#134)
- Serialize standalone comments with surrounding white-space.
- Allow blank lines inside of messages. (#76)
- Trim trailing newline from Comments. (#77)
Implement projectfluent/fluent#52.