requireMatchingFunctionName: requires function names to match member and property names #850

xaka · 2014-12-22T00:53:48Z

Example of member name mismatch:

var test = {};
test.foo = function bar() {};

Example of property name mismatch:

var test = {foo: function bar() {}};

Fixes #846
Closes #850

… function names Example of member name mismatch: ```js function Test() {}; Test.prototype.foo = function bar() {}; ``` Example of property name mismatch: ```js var test = {foo: function bar() {}}; ``` Fixes jscs-dev#846 Closes jscs-dev#850

coveralls · 2014-12-22T00:56:10Z

Coverage increased (+0.03%) when pulling e491f51 on xaka:require-function-name-match into 9aae17e on jscs-dev:master.

coveralls · 2014-12-22T00:58:17Z

Coverage increased (+0.03%) when pulling 083c63a on xaka:require-function-name-match into 9aae17e on jscs-dev:master.

… function names Example of member name mismatch: ```js function Test() {}; Test.prototype.foo = function bar() {}; ``` Example of property name mismatch: ```js var test = {foo: function bar() {}}; ``` Fixes jscs-dev#846 Closes jscs-dev#850

coveralls · 2014-12-22T01:05:15Z

Coverage increased (+0.03%) when pulling f54d69a on xaka:require-function-name-match into 9aae17e on jscs-dev:master.

coveralls · 2014-12-22T01:06:23Z

Coverage increased (+0.03%) when pulling f54d69a on xaka:require-function-name-match into 9aae17e on jscs-dev:master.

mrjoelkemp · 2014-12-22T14:43:25Z

lib/rules/require-function-name-match.js

+
+function checkForMember(assignment, errors) {
+    // anonymous function OR names mismatch
+    if (!assignment.right.id || (assignment.left.property.name !== assignment.right.id.name)) {


I think the first clause mandates too much. This rule now enforces disallowAnonymousFunctions and requireFunctionNameMatch.

What do you think about this rule enforcing that if a function expression is named, its name must match the assignment property or member?

mrjoelkemp · 2014-12-22T14:45:11Z

Great work @xaka! This is a very clear implementation. I left one comment on the scope of what this rule handles. Once that's addressed, it looks good to land; I'll wait for another reviewer to give the +1 before merging.

mikesherov · 2014-12-22T15:28:05Z

requireMatchingFunctionName would be more English readable IMO. Let's rename before we land.

I'm also concerned that this rule is bloat, which isn't to say it's not a good rule, but now that we have a sane plugin infra we should be more discriminating.

mikesherov · 2014-12-22T15:35:22Z

Also, what if the property name is a reserved word. Named function expressions can't have those as function names while properties can, right?

xaka · 2014-12-22T16:44:01Z

The problem with requireAnonymousFunctions and disallowAnonymousFunctions is that it's either everything or nothing, and there is no middle ground. For me personally there should be no anonymous functions allowed in function expressions, but at the same time I don't care much about anonymous functions for callbacks and events handling. I think the solution would be another rule like disallowAnonymousFunctionExpressions. Would you agree? I'll update the code and remove any checks for anonymous functions for now.
requireMatchingFunctionName sounds even better, will do
Good point about reserved words. What would be the appropriate solution here? Do we have a list of reserved words available at code level so when they are in use, we'll be less restrictive? If so, what about allowing to use $ as a prefix when reserved word is detected?

Thanks for constructive feedback!

…tch function names Example of member name mismatch: ```js function Test() {}; Test.prototype.foo = function bar() {}; ``` Example of property name mismatch: ```js var test = {foo: function bar() {}}; ``` Fixes jscs-dev#846 Closes jscs-dev#850

coveralls · 2014-12-22T16:56:17Z

Coverage increased (+0.04%) when pulling b636959 on xaka:require-function-name-match into 9aae17e on jscs-dev:master.

…tch function names Example of member name mismatch: ```js function Test() {}; Test.prototype.foo = function bar() {}; ``` Example of property name mismatch: ```js var test = {foo: function bar() {}}; ``` Fixes jscs-dev#846 Closes jscs-dev#850

xaka · 2014-12-22T16:59:18Z

I've updated the code. I wish I could change the branch name w/o opening another PR so for now it'll be the only mismatching place.

coveralls · 2014-12-22T17:01:03Z

Coverage increased (+0.04%) when pulling 79a6499 on xaka:require-function-name-match into 9aae17e on jscs-dev:master.

mrjoelkemp · 2014-12-23T16:07:50Z

We have some helpful lists in lib/utils that should help identify reserved words.

…and property names Example of member name mismatch: ```js var test = {}; test.foo = function bar() {}; ``` Example of property name mismatch: ```js var test = {foo: function bar() {}}; ``` Fixes jscs-dev#846 Closes jscs-dev#850

coveralls · 2014-12-23T17:12:38Z

Coverage increased (+0.05%) when pulling a2e6501 on xaka:require-function-name-match into f2496f4 on jscs-dev:master.

xaka · 2014-12-23T17:13:40Z

I've updated the code and documentation. So now when reserved work is detected, we just skip it. I've also added a suggestion to prefix such names with $ instead of enforcing it when the rule is on. Personally I'd enforce such change, but we can leave it for next iteration.

mikesherov · 2014-12-23T17:28:02Z

@xaka, thanks again for contributing. Please remove the $ prefix suggestion language from the rule, as I think it's best to let the user decide for best to mitigate the problem.

…and property names Example of member name mismatch: ```js var test = {}; test.foo = function bar() {}; ``` Example of property name mismatch: ```js var test = {foo: function bar() {}}; ``` Fixes jscs-dev#846 Closes jscs-dev#850

xaka · 2014-12-23T17:31:02Z

@mikesherov done, sir!

coveralls · 2014-12-23T17:33:12Z

Coverage increased (+0.05%) when pulling 86f85fb on xaka:require-function-name-match into f2496f4 on jscs-dev:master.

mikesherov · 2015-01-27T16:42:23Z

lib/rules/require-matching-function-name.js

+                // object.foo = function bar() {}
+                // object['foo'] = function bar() {}
+                case 'AssignmentExpression':
+                    checker = checkForMember;


just call checkForMember(node.parentNode, errors); here;

mikesherov · 2015-01-27T16:44:44Z

@xaka just a couple of more comments and a rebase should bring this one across the finish line. Thanks again for contributing!

…and property names Example of member name mismatch: ```js var test = {}; test.foo = function bar() {}; ``` Example of property name mismatch: ```js var test = {foo: function bar() {}}; ``` Fixes jscs-dev#846 Closes jscs-dev#850

xaka · 2015-01-27T17:30:24Z

@mikesherov Done. Please double-check the documentation part as you changed the way it's generated and I did my best to follow the same path.

coveralls · 2015-01-27T17:37:33Z

Coverage increased (+0.01%) to 98.74% when pulling 2b52452 on xaka:require-function-name-match into 87a382d on jscs-dev:master.

markelog · 2015-06-10T17:42:08Z

@mikesherov are we good here?

markelog · 2015-06-12T18:16:15Z

Thank you for the patience! And sorry it took that long to land.

Requires function names to match member and property names Example of member name mismatch: ```js var test = {}; test.foo = function bar() {}; ``` Example of property name mismatch: ```js var test = {foo: function bar() {}}; ``` Fixes jscs-dev#846 Closes jscs-dev#850

xaka force-pushed the require-function-name-match branch from e491f51 to 083c63a Compare December 22, 2014 00:55

xaka force-pushed the require-function-name-match branch from 083c63a to 6c57462 Compare December 22, 2014 01:02

xaka force-pushed the require-function-name-match branch from 6c57462 to f54d69a Compare December 22, 2014 01:03

mrjoelkemp reviewed Dec 22, 2014
View reviewed changes

xaka force-pushed the require-function-name-match branch from f54d69a to b636959 Compare December 22, 2014 16:53

xaka changed the title ~~requireFunctionNameMatch: requires member and property names to match function names~~ requireMatchingFunctionName: requires member and property names to match function names Dec 22, 2014

xaka force-pushed the require-function-name-match branch from b636959 to 6422e23 Compare December 22, 2014 16:56

xaka force-pushed the require-function-name-match branch from 6422e23 to 79a6499 Compare December 22, 2014 16:57

xaka force-pushed the require-function-name-match branch from 79a6499 to a2e6501 Compare December 23, 2014 17:09

xaka changed the title ~~requireMatchingFunctionName: requires member and property names to match function names~~ requireMatchingFunctionName: requires function names to match member and property names Dec 23, 2014

xaka force-pushed the require-function-name-match branch from a2e6501 to 86f85fb Compare December 23, 2014 17:29

markelog added the new rule label Jan 3, 2015

markelog force-pushed the master branch from 2edad7b to 3bdb4d5 Compare January 15, 2015 05:10

mikesherov reviewed Jan 27, 2015
View reviewed changes

xaka force-pushed the require-function-name-match branch from 86f85fb to 4749849 Compare January 27, 2015 17:11

xaka force-pushed the require-function-name-match branch from 4749849 to 2b52452 Compare January 27, 2015 17:27

hzoo mentioned this pull request Feb 17, 2015

Preset: Idiomatic #1069

Closed

markelog added this to the 2.0 milestone Jun 11, 2015

markelog closed this in bbd7337 Jun 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

requireMatchingFunctionName: requires function names to match member and property names #850

requireMatchingFunctionName: requires function names to match member and property names #850

xaka commented Dec 22, 2014

coveralls commented Dec 22, 2014

coveralls commented Dec 22, 2014

coveralls commented Dec 22, 2014

coveralls commented Dec 22, 2014

mrjoelkemp Dec 22, 2014

mrjoelkemp commented Dec 22, 2014

mikesherov commented Dec 22, 2014

mikesherov commented Dec 22, 2014

xaka commented Dec 22, 2014

coveralls commented Dec 22, 2014

xaka commented Dec 22, 2014

coveralls commented Dec 22, 2014

mrjoelkemp commented Dec 23, 2014

coveralls commented Dec 23, 2014

xaka commented Dec 23, 2014

mikesherov commented Dec 23, 2014

xaka commented Dec 23, 2014

coveralls commented Dec 23, 2014

mikesherov Jan 27, 2015

mikesherov commented Jan 27, 2015

xaka commented Jan 27, 2015

coveralls commented Jan 27, 2015

markelog commented Jun 10, 2015

markelog commented Jun 12, 2015

requireMatchingFunctionName: requires function names to match member and property names #850

requireMatchingFunctionName: requires function names to match member and property names #850

Conversation

xaka commented Dec 22, 2014

coveralls commented Dec 22, 2014

coveralls commented Dec 22, 2014

coveralls commented Dec 22, 2014

coveralls commented Dec 22, 2014

mrjoelkemp Dec 22, 2014

Choose a reason for hiding this comment

mrjoelkemp commented Dec 22, 2014

mikesherov commented Dec 22, 2014

mikesherov commented Dec 22, 2014

xaka commented Dec 22, 2014

coveralls commented Dec 22, 2014

xaka commented Dec 22, 2014

coveralls commented Dec 22, 2014

mrjoelkemp commented Dec 23, 2014

coveralls commented Dec 23, 2014

xaka commented Dec 23, 2014

mikesherov commented Dec 23, 2014

xaka commented Dec 23, 2014

coveralls commented Dec 23, 2014

mikesherov Jan 27, 2015

Choose a reason for hiding this comment

mikesherov commented Jan 27, 2015

xaka commented Jan 27, 2015

coveralls commented Jan 27, 2015

markelog commented Jun 10, 2015

markelog commented Jun 12, 2015