Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Cleanup and splitup parser functions #295

Merged
merged 1 commit into from
Jan 20, 2017
Merged

Cleanup and splitup parser functions #295

merged 1 commit into from
Jan 20, 2017

Conversation

danez
Copy link
Member

@danez danez commented Jan 14, 2017

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

This makes it easier to integrate plugins like the estree plugin. It was splitted out of #277 to make it easier to review.

No changes to the logic, just purely refactor.

Added inline comments, so it is easier to follow what has happened

@codecov-io
Copy link

codecov-io commented Jan 14, 2017

Current coverage is 97.27% (diff: 100%)

Merging #295 into master will increase coverage by 0.14%

@@             master       #295   diff @@
==========================================
  Files            21         21          
  Lines          3487       3489     +2   
  Methods         400        406     +6   
  Messages          0          0          
  Branches        896        900     +4   
==========================================
+ Hits           3387       3394     +7   
+ Misses           44         40     -4   
+ Partials         56         55     -1   

Powered by Codecov. Last update 0a00aff...df858ab

@danez danez requested review from loganfsmyth and DrewML January 16, 2017 09:22
@@ -30,26 +30,13 @@ const pp = Parser.prototype;
// strict mode, init properties are also not allowed to be repeated.

pp.checkPropClash = function (prop, propHash) {
if (prop.computed) return;
if (prop.computed || prop.kind) return;
Copy link
Member Author

Choose a reason for hiding this comment

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

moved from line 39, to exit earlier

return;
}
// It is either an Identifier or a String/NumericLiteral
const name = key.type === "Identifier" ? key.name : String(key.value);
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of doing a switch case with two variant just do this one-liner. It will anyway only be Identifier or Literal.

this.raise(start, "setter should have exactly one param");
}
}
};
Copy link
Member Author

Choose a reason for hiding this comment

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

New method to check if getters and setter have correct param count. Was duplicated in the code twice.

@@ -808,36 +795,57 @@ pp.parseObj = function (isPattern, refShorthandDefaultPos) {
return this.finishNode(node, isPattern ? "ObjectPattern" : "ObjectExpression");
};

pp.parseObjPropValue = function (prop, startPos, startLoc, isGenerator, isAsync, isPattern, refShorthandDefaultPos) {
pp.isGetterOrSetterMethod = function (prop, isPattern) {
Copy link
Member Author

Choose a reason for hiding this comment

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

extracted from parseObjectMethod to be more easier to follow.

@@ -808,36 +795,57 @@ pp.parseObj = function (isPattern, refShorthandDefaultPos) {
return this.finishNode(node, isPattern ? "ObjectPattern" : "ObjectExpression");
};

pp.parseObjPropValue = function (prop, startPos, startLoc, isGenerator, isAsync, isPattern, refShorthandDefaultPos) {
Copy link
Member Author

Choose a reason for hiding this comment

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

parseObjPropValue was split into parseObjectMethod and parseObjectProperty

this.raise(start, "setter should have exactly one param");
}
}
this.parseMethod(prop);
Copy link
Member Author

Choose a reason for hiding this comment

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

removed second param, as now not needed (see comment in parseMethod)

return stmt.type === "ExpressionStatement" &&
stmt.expression.type === "StringLiteral" &&
!stmt.expression.extra.parenthesized;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

extracted for readability and to be overwriteable for estree

(key.type === "Identifier" && key.name === "prototype") ||
(key.type === "StringLiteral" && key.value === "prototype")
(key.name === "prototype") || // Identifier
(key.value === "prototype") // Literal
Copy link
Member Author

Choose a reason for hiding this comment

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

no need to check the types, just check the name/value. Helps with estree (StringLiteral != Literal)

} else {
this.raise(start, "setter should have exactly one param");
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to checkGetterSetterParamCount

} else {
this.raise(start, "setter should have exactly one param");
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to checkGetterSetterParamCount

This makes it easier to integrate the estree plugin.
@danez danez merged commit b6c3b5a into master Jan 20, 2017
@danez danez deleted the cleanup branch January 20, 2017 22:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants