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

Require function names to match the name of the variable or property to which they are assigned #665

Closed
feross opened this issue Oct 21, 2016 · 12 comments

Comments

@feross
Copy link
Member

commented Oct 21, 2016

This rule requires function names to match the name of the variable or property to which they are assigned.

http://eslint.org/docs/rules/func-name-matching

@feross feross added the enhancement label Oct 21, 2016

@mightyiam

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2016

I can see how this can prevent hair-loss in debugging.

@timoxley

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2016

Bitten by this. "How the shit is that function even being called, that's not even the right line number..." then going off trying to figure out what's wrong with my source maps... only to notice later they were just fine, the bug was something else entirely and I had just wasted an hour because I'd copy/pasted a function signature but forgot to rename it later. +1.

@dougwilson

This comment has been minimized.

Copy link

commented Oct 21, 2016

This change seem cool to me. I try to keep the function names matching the method name when assigning to the prototype (old school, I know), but it's easy to make mistakes.

@jprichardson

This comment has been minimized.

Copy link
Member

commented Oct 21, 2016

On the surface this one seems great... but will it screw with monkey patching? What about bind? If so, are there viable work arounds or recommended alternatives?

e.g.

let oldLog = console.log
console.log = function (msg) {
  console.log('patched!')
  oldLog(msg)
}

Will that work? or will I have to name it let log?

Also, what about using bind? That changes the function name IRC.

@dougwilson

This comment has been minimized.

Copy link

commented Oct 21, 2016

Your example and .bind work just fine what that eslint rule and are allowed, afaik.

@Flet

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

For fun, ran this rule against the standard-packages set used in tests.

func-name-matching % Pass
["error"] 94.2% (310/329 repos)
["error", { "includeCommonJSModuleExports": true }] 79.9% (263/329 repos)
@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2016

I think I'm 👎 on this change, this is problematic for the following case:

module.exports = function () {
  return {
    class: createClass,
  }
}

// 'class' is a reserved word, so thus cannot be used
// as a function name
function createClass () {
}

This would force authors to either:

  1. adopt ES6 and use the { [class]: createClass } syntax
  2. change their API, forcing semver major

Both of which are not necessarily an option - the only option would then be to disable it in specific locations.

I'd be fine with this rule being enabled if it solved a problem, but to my knowledge I've never run into this before so I don't feel the costs outweigh the gains.

Also in the case of ["error", { "includeCommonJSModuleExports": true }] it actually makes stack traces worse as functions are no longer named - and we can no longer hoist functions for the common pattern of:

const foo = require('bar')

module.exports = mainView

// description of what it does
function mainView () {
}

Thoughts?

@Flet

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Both of those cases pass this rule without issue @yoshuawuyts 👍

This does not pass:

var foo = require('bar')
module.exports.foo = function mainView () {}

Strangely, this one also passes just fine:

var foo = require('bar')
module.exports.foo = mainView
function mainView () {}
@Flet

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

...and for good measure... this also fails:

module.exports = function () {
  return {
    class: function createClass () {}
  }
}
@dcousens

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

ACK

@feross feross added this to the standard v10 milestone Feb 9, 2017

@caesarsol

This comment has been minimized.

Copy link

commented Feb 9, 2017

Yeah, the rule works only on functions defined inline on an object property or property reassign.

Is this going to completely forbid anonymous functions as object property? Because I'd love it for debugging :)

👍

@feross

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2017

Moderate impact on ecosystem (~5%). Not automatically fixable.

# tests 287
# pass  273
# fail  14

There also seem to be some cases that are forbidden but that seem reasonable to me:

Case 1: exposing a method with a short name like require('get-installed-path').sync, but giving it a more descriptive function name:

module.exports.sync = function getInstalledPathSync (name, opts) {
module.exports.sync = function isInstalledSync (name) {
round.down = function roundDown (value, multiple) {
exports.es3 = function fillKeysEs3 (destination, source) {
Scale.create = function Scale$create (prefixesList, base, initExp) {

Case 2: setting function as a generic property name like command, required by certain APIs, but giving the function a more descriptive name:

{
  command: function cat (args) {

Case 3: naming a function differently to prevent shadowing:

function humanizer (passedOptions) {
  var result = function humanizer (ms, humanizerOptions) {
    var options = extend({}, result, humanizerOptions || {})
    return doHumanization(ms, options)
  }

There are enough legit cases that I'm hesitant to include this in v10. If anyone who argued for this rule disagrees, feel free to continue discussion below. I'll close this for now.

@feross feross closed this Mar 1, 2017

@feross feross removed this from the standard v10 milestone Mar 1, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
9 participants
You can’t perform that action at this time.