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

Consistency in enforcing variable names? #550

Closed
zaynv opened this issue Jun 18, 2016 · 11 comments

Comments

@zaynv
Copy link

commented Jun 18, 2016

Just wanted to see if you had any thoughts on making the variable name enforcing a bit more consistent.

The rule called handle-callback-err seems to throw an error when you have a variable called err or error in your callback and you don't handle it. Example:

import fs from 'fs'

fs.readFile('./hi.txt', (err, file) => {
  console.log(file.toString())
})

The same thing happens if you use error as the variable name, but not anything else. So it looks like it is pushing you to use one of those variables.

With Promises, there seems to be an inconsistency. When you use resolve and reject as the variable names, everything works fine:

import fs = from 'fs'

function readFile (path) {
  return new Promise((resolve, reject) => {
    fs.readFile(path, (err, file) => {
      if (err) reject(err)
      resolve(file)
    })
  })
}

readFile('./hi.txt')
  .then(file => {
    console.log(file.toString())
  })
  .catch(err => {
    console.error(err.stack)
  })

But if you use the three-letter shorthands res and rej, (similar to the error vs err), you get a linting error from the promise/param-names rule that says "Promise constructor parameters must be named resolve, reject".

Is it possible to make this more consistent? I don't really mind which one is used, but it would be preferable to enforce error instead of err or otherwise allow err and then also allow res and rej to be used.

@ifraixedes

This comment has been minimized.

Copy link

commented Jun 18, 2016

I would prefer to enforce one name for the error callback functions, which should be error to be consistent with the current Promise rule; although I guess that forcing error name will break a lot of projects.

@LinusU

This comment has been minimized.

Copy link
Member

commented Jun 18, 2016

I would personally prefer err since this is what the vast majority of the ecosystem is using. Anyhow, it doesn't matter since it will be impossible to enforce, we cannot know if the first parameter will contain an error or not. Consider the following two examples, how should standard warn?

function example1 (fn) {
  fn(null, 1)
}

function example2 (fn) {
  fn(2)
}

example1(function (theError, theValue) { console.log(theValue) })
example2(function (theValue) { console.log(theValue) })
@feross

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

Right now, there's no ESLint rule for enforcing the name of the error callback. But we still want to detect if the programmer forgets to handle the error, so we're erring on the side of caution and ensuring that variables named err or error are handled.

In the future, if there's some way to enforce that err is used, and not error, then we'll do that.

@feross

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

As for the promise function names res and rej, I believe that @timoxley prefers these names. Is this still the case, @timoxley?

I tend to not use promises, so I'm not sure what the majority of the community is using these days. For promises, we do have the ability to enforce which function names are used, and I agree that res and rej are indeed more consistent with err.

Do any promise users have thoughts on this?

@feross feross added the question label Jul 13, 2016

@LinusU

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

I use promises almost every day and I don't think I've ever come across res and rej. I also prefer the longer variant because it feels more explicit.

@feross

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

Closing due to lack of feedback. There's nothing actionable here.

@feross feross closed this Aug 19, 2016

@timoxley

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2016

As for the promise function names res and rej, I believe that @timoxley prefers these names. Is this still the case, @timoxley?

@feross missed this, apologies.

The comment where I mentioned this was suggesting that perhaps standard should hold off on defining anything until the community has more time to experiment and come to a defacto standard.

Explicit is nice at first but grows unnecessarily verbose as the community grows a shared understanding e.g. req and res from express/connect.

Also, somewhat in jest I've been preferring yeah and nah since rej and res look very similar on first glance; though perhaps the same could be said of req and res. i.e. I'd prefer accept/reject over resolve/reject for readability even though the latter is more correct.

@dcousens

This comment has been minimized.

Copy link
Member

commented Aug 20, 2016

yeah, nah... even as an Australian, I still don't know which one is rej and res.

@trusktr

This comment has been minimized.

Copy link

commented Apr 17, 2018

Here's a reason why we should allow res, rej:

function createPromise() {

    let resolve = () => {}
    let reject = () => {}

    const promise = new Promise( ( res, rej ) => { resolve = res; reject = rej } )

    return { promise, resolve, reject }

}

If I name them resolve and reject, then what do I name the outer scope ones?

@trusktr

This comment has been minimized.

Copy link

commented Apr 17, 2018

This helper makes it easy to write other code like this:

function increaseWidthBy( selector: string, px: number ) {

    const { promise, resolve } = createPromise()

    window.addEventListener( 'load', () => {

        addWidth( links, px )
        resolve()

    } )

    return promise

}

without having to write

    let resolve = () => {}
    let reject = () => {}
    const promise = new Promise( ( res, rej ) => { resolve = res; reject = rej } )

all the time, and without having to nest code inside the executor function passed to new Promise.

@LinusU

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@trusktr Whats wrong with having the code inside the constructor?

function increaseWidthBy( selector: string, px: number ) {
  return new Promise((resolve) => {
    window.addEventListener('load', () => {
      addWidth(links, px)
      resolve()
    })
  })
}

The idea with the revealing constructor pattern is that resolve and reject should not leak outside of the Promise. I personally have ran into very very few cases where I actually needed to expose those outside, and in most of those cases, they where being put on objects so naming the parameters wasn't a problem.

If you are running into this often, I think that you might be doing something that's not quite aligned with how others are doing it, and in that case I don't think that standard should change any rules.

In your example, you also want to name the parameters resolve and reject so I personally think a better workaround for you would be something like:

let resolve = () => {}
let reject = () => {}
const promise = new Promise((...args) => { resolve = args[0]; reject = args[1] })

tl;dr I think that enforcing resolve and reject has worked very good, and improves code quality. I would personally not want to relax this rule...

@lock lock bot locked as resolved and limited conversation to collaborators Jul 16, 2018

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