Skip to content

Add 0.5 comments support #109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 11, 2018
Merged

Add 0.5 comments support #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions fluent-syntax/src/ast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,10 @@ class SyntaxNode extends BaseNode {
}

export class Resource extends SyntaxNode {
constructor(body = [], comment = null) {
constructor(body = []) {
super();
this.type = 'Resource';
this.body = body;
this.comment = comment;
}
}

Expand Down Expand Up @@ -192,20 +191,31 @@ export class Symbol extends Identifier {
}
}

export class Comment extends Entry {
export class BaseComment extends Entry {
constructor(content) {
super();
this.type = 'Comment';
this.type = 'BaseComment';
this.content = content;
}
}

export class Section extends Entry {
constructor(name, comment = null) {
super();
this.type = 'Section';
this.name = name;
this.comment = comment;
export class Comment extends BaseComment {
constructor(content) {
super(content);
this.type = 'Comment';
}
}

export class GroupComment extends BaseComment {
constructor(content) {
super(content);
this.type = 'GroupComment';
}
}
export class ResourceComment extends BaseComment {
constructor(content) {
super(content);
this.type = 'ResourceComment';
}
}

Expand Down
35 changes: 34 additions & 1 deletion fluent-syntax/src/ftlstream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export class FTLParserStream extends ParserStream {
return ((cc >= 48 && cc <= 57) || cc === 45); // 0-9
}

isPeekNextLineComment() {
isPeekNextLineZeroFourStyleComment() {
if (!this.currentPeekIs('\n')) {
return false;
}
Expand All @@ -128,6 +128,39 @@ export class FTLParserStream extends ParserStream {
return false;
}

// -1 - any
// 0 - comment
// 1 - group comment
// 2 - resource comment
isPeekNextLineComment(level = -1) {
if (!this.currentPeekIs('\n')) {
return false;
}

let i = 0;

while (i <= level || (level === -1 && i < 3)) {
this.peek();
if (!this.currentPeekIs('#')) {
if (i !== level && level !== -1) {
this.resetPeek();
return false;
}
break;
}
i++;
}

this.peek();
if ([' ', '\n'].includes(this.currentPeek())) {
this.resetPeek();
return true;
}

this.resetPeek();
return false;
}

isPeekNextLineVariantStart() {
if (!this.currentPeekIs('\n')) {
return false;
Expand Down
100 changes: 84 additions & 16 deletions fluent-syntax/src/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ function withSpan(fn) {
}

// Spans of Messages and Sections should include the attached Comment.
if (node.type === 'Message' || node.type === 'Section') {
if (node.type === 'Message') {
if (node.comment !== null) {
start = node.comment.span.start;
}
Expand All @@ -42,7 +42,7 @@ export default class FluentParser {

// Poor man's decorators.
[
'getComment', 'getSection', 'getMessage', 'getAttribute',
'getComment', 'getMessage', 'getAttribute',
'getIdentifier', 'getVariant', 'getSymbol', 'getNumber', 'getPattern',
'getTextElement', 'getPlaceable', 'getExpression',
'getSelectorExpression', 'getCallArg', 'getString', 'getLiteral',
Expand All @@ -52,8 +52,6 @@ export default class FluentParser {
}

parse(source) {
let comment = null;

const ps = new FTLParserStream(source);
ps.skipBlankLines();

Expand All @@ -62,8 +60,16 @@ export default class FluentParser {
while (ps.current()) {
const entry = this.getEntryOrJunk(ps);

if (entry.type === 'Comment' && entries.length === 0) {
comment = entry;
if (entry === null) {
// That happens when we get a 0.4 style section
continue;
}

if (entry.type === 'Comment' &&
entry.zeroFourStyle && entries.length === 0) {
const comment = new AST.ResourceComment(entry.content);
comment.span = entry.span;
entries.push(comment);
} else {
entries.push(entry);
}
Expand All @@ -72,7 +78,7 @@ export default class FluentParser {
ps.skipBlankLines();
}

const res = new AST.Resource(entries, comment);
const res = new AST.Resource(entries);

if (this.withSpans) {
res.addSpan(0, ps.getIndex());
Expand Down Expand Up @@ -117,7 +123,7 @@ export default class FluentParser {
getEntry(ps) {
let comment;

if (ps.currentIs('/')) {
if (ps.currentIs('/') || ps.currentIs('#')) {
comment = this.getComment(ps);

// The Comment content doesn't include the trailing newline. Consume
Expand All @@ -127,20 +133,25 @@ export default class FluentParser {
}

if (ps.currentIs('[')) {
return this.getSection(ps, comment);
this.skipSection(ps);
if (comment) {
return new AST.GroupComment(comment.content);
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but if you bring back the previous order of ifs, so first test for [ and then test for identifier start, you could get away without this return null here and just let the code flow to the next if.

You could then remove the node === null check in withSpan and parse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and just let the code flow to the next if.

Then it'll return regular Comment for old style comment that was attached to a section, instead of GroupComment.

You could then remove the node === null check in withSpan and parse.

That's crashes if getEntry returns null (it does when section doesn't have a comment in 0.4 style).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean something like this:

  getEntry(ps) {
    let comment;

    if (ps.currentIs('/') || ps.currentIs('#')) {
      comment = this.getComment(ps);

      // The Comment content doesn't include the trailing newline. Consume
      // this newline here to be ready for the next entry.  undefined stands
      // for EOF.
      ps.expectChar(ps.current() ? '\n' : undefined);
    }

    if (ps.currentIs('[')) {
      this.skipSection(ps);
      if (comment) {
        return new AST.GroupComment(comment.content);
      }
    }

    if (ps.isIDStart() && (!comment || comment.type === 'Comment')) {
      return this.getMessage(ps, comment);
    }

    if (comment) {
      return comment;
    }

    throw new ParseError('E0002');
  }

Then you never return null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do in case the file is:

[[ This is a section ]]

which is one of our tests :(

What do you prefer here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point, thanks. Let's keep the tests.

Can we at least remove the node === null check in withSpans? I think we only return null from getEntry which isn't decorated with withSpans because it handles spans in an own custom way.

}

if (ps.isIDStart()) {
if (ps.isIDStart() && (!comment || comment.type === 'Comment')) {
return this.getMessage(ps, comment);
}

if (comment) {
return comment;
}

throw new ParseError('E0002');
}

getComment(ps) {
getZeroFourStyleComment(ps) {
ps.expectChar('/');
ps.expectChar('/');
ps.takeCharIf(' ');
Expand All @@ -153,7 +164,7 @@ export default class FluentParser {
content += ch;
}

if (ps.isPeekNextLineComment()) {
if (ps.isPeekNextLineZeroFourStyleComment()) {
content += '\n';
ps.next();
ps.expectChar('/');
Expand All @@ -163,23 +174,80 @@ export default class FluentParser {
break;
}
}
return new AST.Comment(content);

const comment = new AST.Comment(content);
comment.zeroFourStyle = true;
return comment;
}

getSection(ps, comment) {
getComment(ps) {
if (ps.currentIs('/')) {
return this.getZeroFourStyleComment(ps);
}

// 0 - comment
// 1 - group comment
// 2 - resource comment
let level = -1;
let content = '';

while (true) {
let i = -1;
while (ps.currentIs('#') && (i < (level === -1 ? 2 : level))) {
ps.next();
i++;
}

if (level === -1) {
level = i;
}

if (!ps.currentIs('\n')) {
ps.expectChar(' ');
let ch;
while ((ch = ps.takeChar(x => x !== '\n'))) {
content += ch;
}
}

if (ps.isPeekNextLineComment(level, false)) {
content += '\n';
ps.next();
} else {
break;
}
}

let Comment;
switch (level) {
case 0:
Comment = AST.Comment;
break;
case 1:
Comment = AST.GroupComment;
break;
case 2:
Comment = AST.ResourceComment;
break;
}
return new Comment(content);
}

skipSection(ps) {
ps.expectChar('[');
ps.expectChar('[');

ps.skipInlineWS();

const symb = this.getSymbol(ps);
this.getSymbol(ps);

ps.skipInlineWS();

ps.expectChar(']');
ps.expectChar(']');

return new AST.Section(symb, comment);
ps.skipInlineWS();
ps.next();
}

getMessage(ps, comment) {
Expand Down
37 changes: 32 additions & 5 deletions fluent-syntax/src/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ function containNewLine(elems) {
export default class FluentSerializer {
constructor({ withJunk = false } = {}) {
this.withJunk = withJunk;
this.hasEntries = false;
}

serialize(resource) {
Expand All @@ -23,11 +24,15 @@ export default class FluentSerializer {
parts.push(
`${serializeComment(resource.comment)}\n\n`
);
this.hasEntries = true;
}

for (const entry of resource.body) {
if (entry.types !== 'Junk' || this.withJunk) {
parts.push(this.serializeEntry(entry));
if (this.hasEntries === false) {
this.hasEntries = true;
}
}
}

Expand All @@ -40,10 +45,21 @@ export default class FluentSerializer {
return serializeMessage(entry);
case 'Section':
return serializeSection(entry);
case 'Comment': {
const comment = serializeComment(entry);
return `\n${comment}\n\n`;
}
case 'Comment':
if (this.hasEntries) {
return `\n${serializeComment(entry)}\n\n`;
}
return `${serializeComment(entry)}\n\n`;
case 'GroupComment':
if (this.hasEntries) {
return `\n${serializeGroupComment(entry)}\n\n`;
}
return `${serializeGroupComment(entry)}\n\n`;
case 'ResourceComment':
if (this.hasEntries) {
return `\n${serializeResourceComment(entry)}\n\n`;
}
return `${serializeResourceComment(entry)}\n\n`;
case 'Junk':
return serializeJunk(entry);
default :
Expand All @@ -55,10 +71,21 @@ export default class FluentSerializer {

function serializeComment(comment) {
return comment.content.split('\n').map(
line => `// ${line}`
line => line.length ? `# ${line}` : '#'
).join('\n');
}

function serializeGroupComment(comment) {
return comment.content.split('\n').map(
line => line.length ? `## ${line}` : '##'
).join('\n');
}

function serializeResourceComment(comment) {
return comment.content.split('\n').map(
line => line.length ? `### ${line}` : '###'
).join('\n');
}

function serializeSection(section) {
const name = serializeSymbol(section.name);
Expand Down
1 change: 0 additions & 1 deletion fluent-syntax/test/fixtures_structure/elements_indent.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@
}
}
],
"comment": null,
"span": {
"type": "Span",
"start": 0,
Expand Down
1 change: 0 additions & 1 deletion fluent-syntax/test/fixtures_structure/message_with_empty_pattern.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
}
}
],
"comment": null,
"span": {
"type": "Span",
"start": 0,
Expand Down
1 change: 0 additions & 1 deletion fluent-syntax/test/fixtures_structure/placeable_at_eol.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,6 @@
}
}
],
"comment": null,
"span": {
"type": "Span",
"start": 0,
Expand Down
Loading