Skip to content
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.

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

pug-lexer loc with source endings #2957

Merged
merged 7 commits into from Feb 15, 2018

Conversation

jbsulli
Copy link
Contributor

@jbsulli jbsulli commented Feb 12, 2018

  • Instead of line/col, there is now a loc object on all tokens that matches the babel loc object (ex: {start:{line:1,column:1},end:{line:1,column:10},filename:"test.pug"}
  • Also assigns mustEscape: true to all boolean attributes (ex: input(disabled))
  • Refactor attrs into three separate functions: attrs, attributeName and attributeValue
  • Update pug-parser and pug to use new loc from lexer tokens

var key = '';
var i;

characterParser.defaultState();
Copy link
Member

Choose a reason for hiding this comment

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

This line is a no-op. You don't need to call defaultState unless you are going to use the state that gets returned.

var col = this.colno;
var line = this.lineno;

characterParser.defaultState();
Copy link
Member

Choose a reason for hiding this comment

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

Again, this line is a no-op

this.colno = col;

tok.val = val;
tok.mustEscape = escapeAttr;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than taking and mutating a token. I feel if would be clearer if this function returned {val, mustEscape, remainingSource}

, str = this.input.substr(1, index-1);


attributeName: function(str){
Copy link
Member

Choose a reason for hiding this comment

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

This function should be called attribute since calling it has the effect of lexing both the name and the value. In the future, we might split out a separate attributeName function.

this.incrementColumn(1);
this.tokEnd(tok);
this.tokens.push(tok);
Copy link
Member

Choose a reason for hiding this comment

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

In most places you have done this.tokens.push(this.tokEnd(tok)); - I think we should be consistent here.

@ForbesLindesay
Copy link
Member

I added some comments to the commit. I think this looks really good, but it's a bit tricky to review because there's so much git merging happening and it makes it hard to figure out exactly what's changed. If you could rebase everything off master so there's only a few commits. Ideally have the commit that updates all the snapshots be a separate commit from the commit with your manual changes to the source code, that would really help, but if that's too much work, then that's ok.

@jbsulli
Copy link
Contributor Author

jbsulli commented Feb 12, 2018

Just updated the commits and removed the merge commits. I will update with your suggestions when I get a chance later today.

@ForbesLindesay
Copy link
Member

Ping me when you get a chance to do those updates.

- remove no-ops
- rename `attributeName` to `attribute`
- attributeName no longer takes a tok and returns a reponse object
- minor cleanup
@jbsulli
Copy link
Contributor Author

jbsulli commented Feb 15, 2018

@ForbesLindesay, I just finished the updates you requested.

For PR #2956, it's easy to default mustEscape to false for boolean attributes. Just change the value here: (packages/pug-lexer/index.js:1122).

@ForbesLindesay ForbesLindesay merged commit 8248ba8 into pugjs:master Feb 15, 2018
@ForbesLindesay
Copy link
Member

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants