Question: how to configure import/order to not enforce any policy on empty lines?

[sompylasar]

My rule is configured as:

"import/order": [ "warn" ],

So I assume this clause should be triggering, as I omit newlines-between:

newlines-between: [always|never]:
...
If omitted, assertion messages will be neither enforced nor forbidden.

But I still get a warning about empty lines:

import/order There should be no empty line between import groups at line 5 col 1

for the following code:

import foo from 'foo-core';

import {
    FooError,
} from 'foo-utils';

I expect no warnings about empty lines if newlines-between is not configured.

Probably it would be nice to have an explicit "ignore" value along with "always" and "never" which will be the default behavior if newslines-between isn't configured.

[jfmengels]

If omitted, assertion messages will be neither enforced nor forbidden.

I think this is just some doc error that I made, this rule doesn't talk about assertion messages at all. At the moment, you can either enforce having an empty line, or enforce having no empty lines, one or the other. Though looking at the code makes me doubt this statement. I'll have to look at it more deeply.

I'm 👍 the idea for an ignore setting value for newlines-between. To have it be the default would be a breaking change, but could be considered for v2. Seems like a sensible default, but I wouldn't mind it being (staying?) an enforcement of no empty lines (go bikeshedding).

[sompylasar]

Personally I don't care about the default as long it can be changed via a configuration value. Currently it can't, because "you can either enforce having an empty line, or enforce having no empty lines, one or the other." which is very unfortunate.

Thanks for the clarification though, I hope this "you can either enforce having an empty line, or enforce having no empty lines, one or the other" will get to the documentation.

[benmosher]

@jfmengels: newlines-between: null could mean "don't enforce"?

[jfmengels]

It could, but we might as well make it a new explicit string value. Clearer and more consistent

[sompylasar]

@jfmengels Speaking about the code, it doesn't even check for "never":

    if (newlinesBetweenImports === 'always') {
      if (currentImport.rank !== previousImport.rank
        && getNumberOfEmptyLinesBetween(currentImport, previousImport) === 0)
      {
        context.report(
          previousImport.node, 'There should be at least one empty line between import groups'
        )
      } else if (currentImport.rank === previousImport.rank
        && getNumberOfEmptyLinesBetween(currentImport, previousImport) > 0)
      {
        context.report(
          previousImport.node, 'There should be no empty line within import group'
        )
      }
    } else {
      if (getNumberOfEmptyLinesBetween(currentImport, previousImport) > 0) {
        context.report(previousImport.node, 'There should be no empty line between import groups')
      }
    }

Tried to follow the code into getNumberOfEmptyLinesBetween and did not understand how exactly it works... stumbled upon rank !== -1 and (type === 'import' ? 0 : 100) which look like fragile magic numbers.

• https://github.com/benmosher/eslint-plugin-import/blob/0a918b1ba138ffb2cb5e864ea2d4c659f197bf3c/src/rules/order.js#L64
• https://github.com/benmosher/eslint-plugin-import/blob/0a918b1ba138ffb2cb5e864ea2d4c659f197bf3c/src/rules/order.js#L69

[benmosher]

ah, I missed this:

Probably it would be nice to have an explicit "ignore" value along with "always" and "never" which will be the default behavior if newslines-between isn't configured.

string is fine 😁

[benmosher]

@sompylasar really laid it on me with that 👎 ... 😅

[sompylasar]

if ('newlines-between' in options) might work if configured as "import/order": [ "warn", {} ], but I haven't verified that. I don't know if ESLint pre-populates the options object with the properties from the options schema.

[jfmengels]

I don't know if ESLint pre-populates the options object with the properties from the options schema.

No it doesn't.

I don't know what you all think, but I think it would be nice to have this option, and that it would be the default setting. If we do want to go that way, that would be a breaking change, so it would have to be included in the upcoming v2 release. Let me know what you think.