[New] include filename in error message #161
Conversation
…into jmm-identify-parent Fix some styling/eslint errors; update planned test cases for tests to pass
Fix conflicts and ensure tests pass
Resolve conflicts resulting from upstream code that does not consider the parent file which may have triggered an error report.
since the error could be specific to a file now.
| @@ -32,22 +32,23 @@ module.exports = function resolve(x, options, callback) { | |||
| var readFile = opts.readFile || fs.readFile; | |||
|
|
|||
| var extensions = opts.extensions || ['.js']; | |||
| var basedir = opts.basedir || path.dirname(caller()); | |||
| var y = opts.basedir || path.dirname(caller()); | |||
There was a problem hiding this comment.
Can this use a better name than "y"? I'd previously renamed this to "basedir" for clarity.
There was a problem hiding this comment.
Hi @ljharb I just basically left @jmm's code intact, but I can certainly change y to basedir instead.
However, note that y is really just a temporary variable and not intended to have any significant meaning, I'm guessing from @jmm's code, because at the end of the day, what you want is, parent and y just splits up the derivation of parent into two parts for clarity.
Anyway, let me know whether you still want y to be basedir and I will make the change.
There was a problem hiding this comment.
At the time they made their PR, the variable was called "y" :-) please do change it.
| @@ -23,25 +23,26 @@ module.exports = function (x, options) { | |||
| var readFileSync = opts.readFileSync || fs.readFileSync; | |||
|
|
|||
| var extensions = opts.extensions || ['.js']; | |||
| var basedir = opts.basedir || path.dirname(caller()); | |||
| var y = opts.basedir || path.dirname(caller()); | |||
|
Closing this PR because we want to have a clean history. |
|
@tcchau i'd have vastly preferred that you rebase instead of deleting the branch and closing the PR; is there any way to reopen this? I'm happy to do the rebase for you if needed. |
|
@ljharb Sorry, I couldn't see how to rebase without force pushing a new master, which prevents re-opening this PR. |
|
@tcchau yes, force pushing a new master would have done it (since the PR came from your master branch). |
|
@ljharb I haven't collaborated on Github in a long time, and forgot that best practice is to code your patch in a branch. I also honestly didn't expect to have to publish what I did since we were waiting for jmm to finish the PR. |
Incorporates changes submitted by @jmm to fix Issue #60, resolves test errors and merges upstream changes. Was originally PR #64.
Fixes #60. Closes #64.