This repository has been archived by the owner. It is now read-only.

Refactor tokenizer types file #263

Merged
merged 9 commits into from Jan 10, 2017

Conversation

Projects
None yet
4 participants
@xtuc
Copy link
Member

xtuc commented Dec 20, 2016

Q A
Bug fix? no
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? no
Tests added/pass? no
Fixed tickets no
License MIT

This is not particularly wanted but it seems clearer to me. It's also to be consistent in usage / definition.

@danez

This comment has been minimized.

Copy link
Member

danez commented Dec 24, 2016

We should keep an eye on performance with changes like this. I created a performance test-suite a while ago, so we should try this maybe. https://github.com/danez/babylon_performance

@xtuc

This comment has been minimized.

Copy link
Member Author

xtuc commented Dec 25, 2016

Yes absolutely.

Diff:

name run
./fixtures/angular.js -18ms
./fixtures/backbone.js -9ms
./fixtures/ember.debug.js -39ms
./fixtures/jquery.js +1ms
./fixtures/react-with-addons.js -15ms

Branch refactor-tokenizer-types:

name run
./fixtures/angular.js 494ms
./fixtures/backbone.js 46ms
./fixtures/ember.debug.js 1223ms
./fixtures/jquery.js 243ms
./fixtures/react-with-addons.js 525ms

Branch master:

name run
./fixtures/angular.js 514ms
./fixtures/backbone.js 55ms
./fixtures/ember.debug.js 1262ms
./fixtures/jquery.js 242ms
./fixtures/react-with-addons.js 510ms

Would be great to include your performance tests in Babylon tests (only on CI). What do you think about it?

@danez

This comment has been minimized.

Copy link
Member

danez commented Dec 27, 2016

I thought about it back then, but wasn't sure how "stable" the performance on different boxes on travis is. But now that I think of it more, maybe if we would run the performance tests twice on travis in the same machine (for master and current commit/branch), the results could be useable.

@@ -45,13 +58,13 @@ export const types = {
eof: new TokenType("eof"),

// Punctuation token types.
bracketL: new TokenType("[", {beforeExpr: true, startsExpr: true}),
bracketL: new TokenType("[", { ...beforeExpr, ...startsExpr }),

This comment has been minimized.

@danez

danez Jan 2, 2017

Member

Seems there are a lot of cases where we have before and startsExpr maybe we can do the spread in this case only once?

const beforeAndStartExpr = { ...beforeExpr, ...startsExpr };

This comment has been minimized.

@xtuc

xtuc Jan 2, 2017

Author Member

Sure, of course. I will update it.

@danharper
Copy link
Member

danharper left a comment

tbh I'm not keen on the beforeExpr & afterExpr var & spreads - seems to harm readability.

@@ -32,10 +37,19 @@ export class TokenType {
}
}

function binop(name, prec) {
return new TokenType(name, {beforeExpr: true, binop: prec});
class KeyworkTokenType extends TokenType {

This comment has been minimized.

@danharper

danharper Jan 8, 2017

Member

typo: KeywordTokenType

xtuc added some commits Jan 8, 2017

@xtuc

This comment has been minimized.

Copy link
Member Author

xtuc commented Jan 8, 2017

@danharper I made some changes to improve the readability. I replaced the spread operators with property shorthands.

@danharper
Copy link
Member

danharper left a comment

lgtm

dot: new TokenType("."),
question: new TokenType("?", beforeExpr),
arrow: new TokenType("=>", beforeExpr),
question: new TokenType("?",{ beforeExpr }),

This comment has been minimized.

@danez

danez Jan 10, 2017

Member

missing space after comma, seems we have no eslint rule active for that

comma: new TokenType(",", { beforeExpr }),
semi: new TokenType(";", { beforeExpr }),
colon: new TokenType(":", { beforeExpr }),
doubleColon: new TokenType("::",{ beforeExpr }),

This comment has been minimized.

@danez

danez Jan 10, 2017

Member

same here

@danez

danez approved these changes Jan 10, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 10, 2017

Current coverage is 97.45% (diff: 100%)

No coverage report found for master at ba96b91.

Powered by Codecov. Last update ba96b91...c65ae91

@danez danez merged commit ed13a4a into babel:master Jan 10, 2017

3 checks passed

codecov/patch 100% of diff hit (target 97.45%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +2.52% compared to cd9aaf2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@xtuc xtuc deleted the xtuc:refactor-tokenizer-types branch Jan 11, 2017

danez pushed a commit that referenced this pull request Jan 12, 2017

Refactor tokenizer types file (#263)
* refactor(tokenizer): use class for keywork tokens

* refactor(tokenizer): re-use constacts in types

* refactor(tokenizer): binop token type

* feat(tokenizer): use beforeAndStartExpr for shortcut

* fix(tokenizer): typo in keywordTokenType

* refactor(tokenizer): don't use spread operator

* refactor(tokenizer): constant for isLoop, isAssign, prefix, postfix

* fix(tokenizer): remove constant beforeAndStartExpr

* style(tokenizer): space after comma
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.