Add `count` option to `newline-after-import` by ntdb · Pull Request #742 · import-js/eslint-plugin-import

ntdb · 2017-02-12T01:45:03Z

My team likes to maintain two newlines after import statements instead of just one. In the interest of making that convention enforceable, these changes add a newlines option to the newline-after-import rule. This option defaults to 1 to maintain full backwards compatibility.

coveralls · 2017-02-12T01:49:47Z

Coverage increased (+0.003%) to 94.915% when pulling 2ced295 on ntdb:newline-after-import into 0800951 on benmosher:master.

ntdb · 2017-02-12T01:50:40Z

That history is super messy, apparently I should have reset my fork before making these changes. Hopefully I can open a new PR without the mess shortly.

coveralls · 2017-02-12T01:53:24Z

Coverage increased (+0.003%) to 94.915% when pulling 2ced295 on ntdb:newline-after-import into 0800951 on benmosher:master.

ljharb · 2017-02-12T02:39:15Z

@ntdb please don't open a new PR; if you rebase this branch on top of the latest master and force push, it should be cleaned up.

coveralls · 2017-02-12T04:23:59Z

Coverage increased (+0.003%) to 94.915% when pulling c3ed126 on ntdb:newline-after-import into 0800951 on benmosher:master.

ljharb · 2017-02-12T04:49:27Z

@ntdb if source is the git remote for this repo, and origin is for your fork, run git fetch source && git checkout master && git reset --hard source/master && git push origin source/master:master -f && git checkout newline-after-import && git rebase source/master && git push origin HEAD:newline-after-import -f.

After that (or amidst that if you get merge conflicts), you may want to edit the changelog so that you're not removing existing entries.

coveralls · 2017-02-12T07:22:15Z

Coverage increased (+0.003%) to 94.915% when pulling 8fe720e on ntdb:newline-after-import into 0800951 on benmosher:master.

ljharb · 2017-02-12T07:29:07Z

CHANGELOG.md


 ## [Unreleased]
-### Added
- [`no-anonymous-default-export`] rule: report anonymous default exports ([#712], thanks [@duncanbeevers]).


Please revert all the changes to this file, except for your addition of course

ljharb · 2017-02-12T07:29:24Z

README.md

@@ -76,7 +76,6 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
 * Limit the maximum number of dependencies a module can have ([`max-dependencies`])
 * Forbid unassigned imports ([`no-unassigned-import`])
 * Forbid named default exports ([`no-named-default`])


please also revert this file.

…test file

ntdb · 2017-02-12T07:34:30Z

I learned some very useful things about git tonight, thanks @ljharb.

coveralls · 2017-02-12T07:36:26Z

Coverage increased (+0.003%) to 94.915% when pulling 8c7ea26 on ntdb:newline-after-import into 0800951 on benmosher:master.

coveralls · 2017-02-12T07:40:41Z

Coverage increased (+0.003%) to 94.915% when pulling 8c7ea26 on ntdb:newline-after-import into 0800951 on benmosher:master.

ljharb

Let's add some test cases for 0 newlines, and an arbitrary number like, say, 4

ljharb · 2017-02-12T07:41:12Z

src/rules/newline-after-import.js

      }

-      if (getLineDifference(node, nextNode) < 2) {
+      const options = context.options[0] || { newlines: 1 }


let's add a rule schema that enforces that newlines is an integer? (ideally a positive one)

…r-import`

coveralls · 2017-02-14T01:27:48Z

Coverage increased (+0.003%) to 94.915% when pulling 1d757dc on ntdb:newline-after-import into 0800951 on benmosher:master.

coveralls · 2017-02-14T01:27:48Z

Coverage increased (+0.003%) to 94.915% when pulling 1d757dc on ntdb:newline-after-import into 0800951 on benmosher:master.

coveralls · 2017-02-14T01:27:48Z

Coverage increased (+0.003%) to 94.915% when pulling 1d757dc on ntdb:newline-after-import into 0800951 on benmosher:master.

ntdb · 2017-02-15T18:17:23Z

@ljharb Thanks, I've added a schema/tests. 👍

duncanbeevers · 2017-02-15T21:31:15Z

@ntdb Nice job on the clean-up.

What do you think changing the option name to count? To me, this seems less repetitive and more explicitly descriptive.

ntdb · 2017-02-16T04:58:25Z

@duncanbeevers I like it. Less repetitive, more explicit, and it sets a usable precedent for other rules with similar options.

coveralls · 2017-02-16T05:34:02Z

Coverage increased (+0.003%) to 94.915% when pulling c0f733b on ntdb:newline-after-import into 0800951 on benmosher:master.

ljharb · 2017-02-16T08:14:33Z

docs/rules/newline-after-import.md

 # newline-after-import

-Enforces having an empty line after the last top-level import statement or require call.
+Enforces having one or more empty line after the last top-level import statement or require call.


"one or more empty lines" (ie, plural)

coveralls · 2017-02-16T19:42:51Z

Coverage increased (+0.003%) to 94.915% when pulling 2323ece on ntdb:newline-after-import into 0800951 on benmosher:master.

jfmengels

LGTM. Thanks a lot for your work @ntdb :D

If you feel contributing again, you could maybe tackle adding an autofix for this rule (that could make your coworkers pretty happy 😄)

jfmengels · 2017-02-16T21:04:59Z

tests/src/rules/newline-after-import.js

-    "var path = require('path');\nvar foo = require('foo');\n",
-    "require('foo');",
-    "switch ('foo') { case 'bar': require('baz'); }",
+    `var path = require('path');\nvar foo = require('foo');\n`,


I don't mind changing this to backticks, but then we might as well replace the \n by in-code line breaks.
(Not necessary for this PR, but if you feel like changing that at some point later, that's would be nice :D).

jfmengels · 2017-02-16T22:04:09Z

Ugh, I just noticed there is already an auto-fixer PR for this rule, which now needs to be updated to handle this case #696, sorry @eelyafi...

coveralls · 2017-02-16T22:51:54Z

Coverage decreased (-1.3%) to 93.616% when pulling 49ea883 on ntdb:newline-after-import into 0800951 on benmosher:master.

coveralls · 2017-02-16T22:57:22Z

Coverage decreased (-1.3%) to 93.616% when pulling 49ea883 on ntdb:newline-after-import into 0800951 on benmosher:master.

ntdb · 2017-02-17T17:44:01Z

@jfmengels I'll open a new PR this weekend to update the auto-fixer. 👍

EDIT: Ah I see it's still in an open PR. I'm still happy to do it but I'll defer to @eelyafi for now.

ljharb added evaluating rule proposal semver-minor labels Feb 12, 2017

ntdb force-pushed the newline-after-import branch from c3ed126 to 8fe720e Compare February 12, 2017 07:17

ljharb requested changes Feb 12, 2017

View reviewed changes

ntdb added 3 commits February 11, 2017 23:31

Add newlines option to newline-after-import

14605f8

Add tests for newline-after-import option and fix linter errors in …

3e5acab

…test file

Fix broken tests, add docs

ba3a5c0

ntdb force-pushed the newline-after-import branch from 8fe720e to ba3a5c0 Compare February 12, 2017 07:32

Update CHANGELOG

8c7ea26

ljharb requested changes Feb 12, 2017

View reviewed changes

Add schema and further testing for newlines option on `newline-afte…

1d757dc

…r-import`

ljharb approved these changes Feb 15, 2017

View reviewed changes

Change newlines option to count for newline-after-import rule

c0f733b

ljharb reviewed Feb 16, 2017

View reviewed changes

Fix missing plural in docs

2323ece

ljharb changed the title ~~Add newlines option to newline-after-import~~ Add count option to newline-after-import Feb 16, 2017

ljharb requested review from benmosher and jfmengels February 16, 2017 19:43

ljharb removed the evaluating label Feb 16, 2017

jfmengels added 2 commits February 16, 2017 22:09

Add links in changelog and thank @ntdb :)

639c227

Add links to rules in changelog

49ea883

jfmengels approved these changes Feb 16, 2017

View reviewed changes

jfmengels merged commit b5a962f into import-js:master Feb 16, 2017

jfmengels mentioned this pull request Feb 16, 2017

Autofixer for newline-after-import. Fixes #686 #696

Merged

ntdb deleted the newline-after-import branch February 19, 2017 22:48

renovate-bot mentioned this pull request May 24, 2017

Update dependency eslint-plugin-import to version 2.3.0 renovatebot/renovate#217

Merged

This was referenced Apr 15, 2018

Update eslint-plugin-import to 2.12.0 sbardian/DebtTrackerAPI#25

Merged

Update dependency eslint-plugin-import to v2.11.0 Urigo/tortilla#68

Merged

Uh oh!

Conversation

ntdb commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntdb commented Feb 12, 2017

Uh oh!

coveralls commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Feb 12, 2017

Uh oh!

coveralls commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

ljharb Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

ntdb commented Feb 12, 2017

Uh oh!

coveralls commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

ljharb Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 14, 2017

Uh oh!

coveralls commented Feb 14, 2017

Uh oh!

coveralls commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntdb commented Feb 15, 2017

Uh oh!

duncanbeevers commented Feb 15, 2017

Uh oh!

ntdb commented Feb 16, 2017

Uh oh!

coveralls commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfmengels left a comment

Choose a reason for hiding this comment

Uh oh!

jfmengels Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

jfmengels commented Feb 16, 2017

Uh oh!

coveralls commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 16, 2017

Uh oh!

ntdb commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ntdb commented Feb 12, 2017 •

edited

Loading

coveralls commented Feb 12, 2017 •

edited

Loading

coveralls commented Feb 12, 2017 •

edited

Loading

coveralls commented Feb 12, 2017 •

edited

Loading

ljharb commented Feb 12, 2017 •

edited

Loading

coveralls commented Feb 12, 2017 •

edited

Loading

coveralls commented Feb 12, 2017 •

edited

Loading

coveralls commented Feb 12, 2017 •

edited

Loading

coveralls commented Feb 14, 2017 •

edited

Loading

coveralls commented Feb 16, 2017 •

edited

Loading

coveralls commented Feb 16, 2017 •

edited

Loading

coveralls commented Feb 16, 2017 •

edited

Loading

ntdb commented Feb 17, 2017 •

edited

Loading