Skip to content
This repository has been archived by the owner on Feb 21, 2022. It is now read-only.

[fix] handle-callback-err (closes #146) #147

Merged
merged 4 commits into from
Dec 14, 2016
Merged

Conversation

jmlopez-rod
Copy link
Collaborator

@jmlopez-rod jmlopez-rod commented Dec 11, 2016

The current code for the rule as it stands has a bug. The line

const highlights = this.languageService.getDocumentHighlights(fileName, parameter.pos, [fileName]);

should be

const highlights = this.languageService.getDocumentHighlights(fileName, parameter.name.getStart(), [fileName]);

Making that change fixes issue #146. In this PR however, I have removed the usage of the languageService. Each call togetDocumentHighlights triggers a function which will search for the parameter in the document tree. The change I made is based on the prefer-arrow-function rule which keeps track of each function scope to determine if we make use of the error parameter. Based on some simple comparisons as noted on the issue we do get some performance with this change.

Manuel Lopez added 3 commits December 10, 2016 00:00
Using a similar technique as in prefer-arrow-functions to determine if
a function uses its first parameter. We keep track of the function
scopes to determine if the parameter is being used.
Added metadata to generate proper docs.
@blakeembrey
Copy link
Collaborator

Looks great! Does this handle the error parameter where it may be handled in a nested function? E.g.

function query (cb) {
  doThing(function (err) {
    closeConnection(function (closeErr) {
      cb(err || closeErr)
    })
  })
})

Checking if it handles the error parameter where it may be handled in a
nested function.
@jmlopez-rod
Copy link
Collaborator Author

@blakeembrey Yup. Seems like a good test to add to the custom regex examples.

@jmlopez-rod jmlopez-rod merged commit 79c7b51 into master Dec 14, 2016
@jmlopez-rod jmlopez-rod deleted the handle-callback-err branch December 14, 2016 04:13
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.

2 participants