eslint-module-utils: filePath in parserOptions #840

sompylasar · 2017-05-19T23:26:47Z

coveralls · 2017-05-19T23:31:42Z

Coverage increased (+0.003%) to 95.062% when pulling b4d75c8 on sompylasar:patch-2 into 1377f55 on benmosher:master.

ljharb

LGTM but it should have a regression test.

sompylasar · 2017-05-20T02:59:06Z

@ljharb OK, will add, what specifically? Assert the path to be actually passed?

ljharb · 2017-05-20T03:19:48Z

@sompylasar that's ideal, yes

Refs import-js#839

sompylasar · 2017-05-20T08:13:35Z

@ljharb

I need a spy to intercept parser call in the test. I've implemented a naive one in 6 lines, but if you'd like to import a library like sinon or chai-spies instead, let me know.
I've bumped the eslint-module-utils version to 2.1.0, added lines to both CHANGELOGs, and updated the eslint-plugin-import dependency version of eslint-module-utils.

sompylasar · 2017-05-20T08:16:48Z

Now CI fail with No compatible version found: eslint-module-utils@^2.1.0, how do you update these versions? @ljharb

ljharb · 2017-05-20T09:02:31Z

utils/CHANGELOG.md

@@ -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


The version number and date aren't decided yet; If you want to call this section "Unreleased" that'd be helpful :-)

ljharb · 2017-05-20T09:02:49Z

utils/package.json

@@ -1,6 +1,6 @@
 {
  "name": "eslint-module-utils",
-  "version": "2.0.0",
+  "version": "2.1.0",


please revert this; package.json should never be bumped in a PR - only in a commit directly made to master.

ljharb · 2017-05-20T09:03:18Z

It'd have to be npm published - but that won't happen until after it's merged.

Refs import-js#839

sompylasar · 2017-05-20T09:11:15Z

@ljharb Reverted: 7712ce1

ljharb

I'd probably bring in sinon (more deps is a good thing), but this is fine for now.

sompylasar · 2017-05-20T09:14:28Z

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...

coveralls · 2017-05-20T09:15:09Z

Coverage increased (+0.05%) to 95.111% when pulling 7712ce1 on sompylasar:patch-2 into 1377f55 on benmosher:master.

benmosher

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

sompylasar · 2017-05-26T21:37:29Z

@benmosher Updated, PTAL. Replaced hand-made spy with sinon.spy.

coveralls · 2017-05-26T21:41:54Z

Coverage increased (+0.04%) to 95.097% when pulling 7ac5e8f on sompylasar:patch-2 into 2f690b4 on benmosher:master.

coveralls · 2017-05-26T21:46:37Z

Coverage increased (+0.04%) to 95.097% when pulling 7ac5e8f on sompylasar:patch-2 into 2f690b4 on benmosher:master.

coveralls · 2017-05-26T22:08:22Z

Coverage increased (+0.07%) to 95.125% when pulling d397b9b on sompylasar:patch-2 into 2f690b4 on benmosher:master.

ljharb · 2017-05-27T03:45:30Z

tests/src/core/parse.js

+  it('passes expected parserOptions to custom parser', function () {
+    const parseSpy = sinon.spy()
+    const parserOptions = { ecmaFeatures: { jsx: true } }
+    require('./parseStubParser').parse = parseSpy


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?

Right, can pull to top. Both this and resolve. Just a habit of having self-contained tests.

Done: 5732742

coveralls · 2017-05-29T23:53:29Z

Coverage increased (+0.07%) to 95.13% when pulling 5732742 on sompylasar:patch-2 into 2f690b4 on benmosher:master.

ljharb · 2017-05-30T02:54:35Z

tests/src/core/parse.js

 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')


requires should generally all only be static and at the top level of the file - can these be moved one level higher?

@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 required 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).

@ljharb For the same reason, the line above: const path = getFilename('jsx.js') could be moved up, but it's not.

I'm not sure I understand, but this can stay where it is.

In other words, require('./parseStubParser') and require.resolve('./parseStubParser') should sit next to each other, they are closely related in this test.

ljharb · 2017-06-02T16:00:35Z

utils/parse.js

@@ -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


Perhaps the path needs to be path.normalized?

Maybe. What is the error case? The serialized parserOptions participates in some cache key, right? Where is it?

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

Sorry, not thinking -- relative would be fine as long as it's all against cwd. So maybe path.normalize would be fine

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

eslint-module-utils: filePath in parserOptions

b4d75c8

Refs import-js#839

ljharb requested changes May 20, 2017

View reviewed changes

eslint-module-utils: Add tests for parserOptions

3544c0f

Refs import-js#839

ljharb requested changes May 20, 2017

View reviewed changes

eslint-module-utils: Reverted manual version bumps.

7712ce1

Refs import-js#839

ljharb approved these changes May 20, 2017

View reviewed changes

ljharb requested review from benmosher and jfmengels May 20, 2017 09:11

ljharb added the semver-minor label May 20, 2017

benmosher requested changes May 24, 2017

View reviewed changes

sompylasar added 3 commits May 26, 2017 14:14

Merge branch 'benmosher/master' into patch-2

d4246fb

* benmosher/master: update CI to build on Node 6+7 (import-js#846) changelog update for 2.3.0

Add sinon, replace eslint-module-utils test spy with sinon.spy

d0007f2

Fix CHANGELOG merge error

7ac5e8f

eslint-module-utils: Add more tests for parse (coverage 100%)

d397b9b

ljharb reviewed May 27, 2017

View reviewed changes

eslint-module-utils: In tests move require stub parser to the top.

5732742

ljharb reviewed May 30, 2017

View reviewed changes

benmosher approved these changes May 31, 2017

View reviewed changes

benmosher merged commit 28e1623 into import-js:master May 31, 2017

renovate-bot mentioned this pull request Jun 2, 2017

Update dependency eslint-plugin-import to version 2.4.0 renovatebot/renovate#227

Closed

ljharb reviewed Jun 2, 2017

View reviewed changes

sompylasar mentioned this pull request Jun 4, 2017

memory leak? #863

Closed

renovate-bot mentioned this pull request Jun 22, 2017

Renovate eslint packages singapore/lint-condo#308

Merged

renovate bot mentioned this pull request Jun 22, 2017

Update dependency eslint-plugin-import to version 2.5.0 renovatebot/renovate#336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eslint-module-utils: filePath in parserOptions #840

eslint-module-utils: filePath in parserOptions #840

sompylasar commented May 19, 2017

coveralls commented May 19, 2017 •

edited

ljharb left a comment

sompylasar commented May 20, 2017

ljharb commented May 20, 2017

sompylasar commented May 20, 2017

sompylasar commented May 20, 2017

ljharb May 20, 2017

ljharb May 20, 2017

ljharb commented May 20, 2017

sompylasar commented May 20, 2017

ljharb left a comment

sompylasar commented May 20, 2017

coveralls commented May 20, 2017 •

edited

benmosher left a comment

sompylasar commented May 26, 2017

coveralls commented May 26, 2017 •

edited

coveralls commented May 26, 2017 •

edited

coveralls commented May 26, 2017 •

edited

ljharb May 27, 2017

sompylasar May 27, 2017

sompylasar May 29, 2017

coveralls commented May 29, 2017 •

edited

ljharb May 30, 2017

sompylasar May 30, 2017

sompylasar May 30, 2017

ljharb May 30, 2017

sompylasar May 30, 2017

ljharb Jun 2, 2017

sompylasar Jun 2, 2017

benmosher Jun 2, 2017

benmosher Jun 2, 2017

sompylasar Jun 2, 2017

eslint-module-utils: filePath in parserOptions #840

eslint-module-utils: filePath in parserOptions #840

Conversation

sompylasar commented May 19, 2017

coveralls commented May 19, 2017 • edited

ljharb left a comment

Choose a reason for hiding this comment

sompylasar commented May 20, 2017

ljharb commented May 20, 2017

sompylasar commented May 20, 2017

sompylasar commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented May 20, 2017

sompylasar commented May 20, 2017

ljharb left a comment

Choose a reason for hiding this comment

sompylasar commented May 20, 2017

coveralls commented May 20, 2017 • edited

benmosher left a comment

Choose a reason for hiding this comment

sompylasar commented May 26, 2017

coveralls commented May 26, 2017 • edited

coveralls commented May 26, 2017 • edited

coveralls commented May 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 19, 2017 •

edited

coveralls commented May 20, 2017 •

edited

coveralls commented May 26, 2017 •

edited

coveralls commented May 26, 2017 •

edited

coveralls commented May 26, 2017 •

edited

coveralls commented May 29, 2017 •

edited