-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
eslint-module-utils: filePath in parserOptions #840
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but it should have a regression test.
@ljharb OK, will add, what specifically? Assert the path to be actually passed? |
@sompylasar that's ideal, yes |
|
Now CI fail with |
utils/CHANGELOG.md
Outdated
@@ -3,9 +3,13 @@ All notable changes to this module will be documented in this file. | |||
This project adheres to [Semantic Versioning](http://semver.org/). | |||
This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com). | |||
|
|||
## v2 - 2016-11-07 | |||
## v2.1.0 - 2017-05-20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version number and date aren't decided yet; If you want to call this section "Unreleased" that'd be helpful :-)
utils/package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "eslint-module-utils", | |||
"version": "2.0.0", | |||
"version": "2.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert this; package.json should never be bumped in a PR - only in a commit directly made to master.
It'd have to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably bring in sinon (more deps is a good thing), but this is fine for now.
👍 I'm for sinon, too. I'd also use chai assert instead of expect (too much magic), but it was already there, so consistency... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual (one-liner) change LGTM 👍🏻
re: tests, I am mildly opposed to adding a handrolled spy to eslint-module-utils
, mostly because it's out of scope for that package. Would rather have a dev-dep on the easiest thing to integrate with.
I'm cool with Sinon.
* benmosher/master: update CI to build on Node 6+7 (import-js#846) changelog update for 2.3.0
@benmosher Updated, PTAL. Replaced hand-made spy with |
tests/src/core/parse.js
Outdated
it('passes expected parserOptions to custom parser', function () { | ||
const parseSpy = sinon.spy() | ||
const parserOptions = { ecmaFeatures: { jsx: true } } | ||
require('./parseStubParser').parse = parseSpy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require has a cache, so you get the same value out of it every time - any reason this isn't required at the top level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, can pull to top. Both this and resolve. Just a habit of having self-contained tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 5732742
import parse from 'eslint-module-utils/parse' | ||
|
||
import { getFilename } from '../utils' | ||
|
||
describe('parse(content, { settings, ecmaFeatures })', function () { | ||
const path = getFilename('jsx.js') | ||
const parseStubParser = require('./parseStubParser') | ||
const parseStubParserPath = require.resolve('./parseStubParser') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires should generally all only be static and at the top level of the file - can these be moved one level higher?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb This require
was left here like that on purpose, this is not just a dependency like any other, this is a stub which must be in a file that can be literally require
d because of the way eslint
, and following it, eslint-import-resolver
, resolves the parser function via require
function (and we can do nothing about that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb For the same reason, the line above: const path = getFilename('jsx.js')
could be moved up, but it's not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand, but this can stay where it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, require('./parseStubParser')
and require.resolve('./parseStubParser')
should sit next to each other, they are closely related in this test.
@@ -22,6 +22,10 @@ exports.default = function parse(path, content, context) { | |||
// always attach comments | |||
parserOptions.attachComment = true | |||
|
|||
// provide the `filePath` like eslint itself does, in `parserOptions` | |||
// https://github.com/eslint/eslint/blob/3ec436ee/lib/linter.js#L637 | |||
parserOptions.filePath = path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the path needs to be path.normalized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. What is the error case? The serialized parserOptions participates in some cache key, right? Where is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The internal parser has a cache that is a hash of the parse settings and the module path. So I'm guessing the path must be relative.
An absolute path would work, or the cache key could drop the file path key.
I am not at a computer right now but I think the cache access is in utils/resolve.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, not thinking -- relative would be fine as long as it's all against cwd. So maybe path.normalize would be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointers, I'll look into that and suggest a change and tests when I get to my computer (afk, too).
…h_in_parser_options * commit '3c46d308ccb462a52554257c49c374045d1a6cf7': rollback utils dependency to 2.0.0 add yank note to utils change log add yanking note to root change log Upgrade debug version of eslint-module-utils (import-js#844) remove obsolete dad joke update utils changelog bump eslint-module-utils to v2.1.0 bump v2.4.0 fix typos, enforce type of array of strings in allow option update CHANGELOG.md eslint-module-utils: filePath in parserOptions (import-js#840) write doc, add two more tests add allow glob for rule no-unassigned-import, fix import-js#671 # Conflicts: # utils/CHANGELOG.md
Refs #839