Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

Added check for es6 methods (harmony) #1020

Closed
wants to merge 1 commit into from
Closed

Added check for es6 methods (harmony) #1020

wants to merge 1 commit into from

Conversation

qfox
Copy link
Member

@qfox qfox commented Feb 8, 2015

Fixes #1013

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.71% when pulling fef0212 on issues/1013 into 394edc9 on master.

@@ -41,7 +41,7 @@ module.exports.prototype = {
check: function(file, errors) {
file.iterateNodesByType('ObjectExpression', function(node) {
node.properties.forEach(function(property) {
if (property.shorthand) {
if (property.shorthand || property.method) {
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a good util: isKeylessProperty(node)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. Property?

Copy link
Member

Choose a reason for hiding this comment

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

We should maybe think about tree iteration yielding custom classes like an ObjectExpression class that has a list of Property instances. Each Property instance will have the isKeylessProperty method. This avoids the bag of utils. Probably an issue worthy comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I really like that idea but realization can be painful. Anyway, we can discuss about it and decide in that issue ;-) So, yep!

@mikesherov
Copy link
Contributor

I see 6 rules changed with only three tests updated. Please add a test to all 6 to prevent coverage regression. If you add the utils method, you'll need to test it in test/utils.js as well.

@mikesherov
Copy link
Contributor

LGTM otherwise.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.0% when pulling 4e70e4c on issues/1013 into fd405cd on master.

@qfox
Copy link
Member Author

qfox commented Feb 8, 2015

About method in utils: I've started to add this but it looks uglier ;-( I suggest to add it as property or method to ObjectExpression node.

@mikesherov
Copy link
Contributor

@zxqfox it's a dangerous path modifying nodes. Let's just stick with what you have for now then, and we can revisit this in the future.

@mikesherov
Copy link
Contributor

LGTM

@mikesherov
Copy link
Contributor

@zxqfox, can you please make sure future commit messages match our documented format from contributing.md:

<rulename>: short message
<emptyline>
Long description (if useful)
<emptyline>
Closes gh-<pullRequestNumber>
Fixes #<issueNumber>

@qfox qfox closed this in bdf0e99 Feb 9, 2015
@qfox qfox deleted the issues/1013 branch February 9, 2015 06:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property '_tokenIndex' of undefined in v.1.11.0
4 participants