Fixes whitespace check-module to properly lint and fix errors

[canufeel]

Fixes whitespace check-module to properly lint and fix errors for import statements with curlies

PR checklist

• Addresses an existing issue: whitespace rule "check-module" option not working as expected #2401
  • Fixes associated tests

Overview of change:

With "whitespace": [true, "check-module"] we should treat the following as errors:

import {A} from './B';
import { A} from './B';
import {A } from './B';
import {A,B} from './B';

and fix those to the following form respectively:

import {A} from './B'; //-> import { A } from './B';
import { A} from './B'; //-> import { A } from './B';
import {A } from './B'; // -> import { A } from './B';
import {A,B} from './B'; // -> import { A, B } from './B';

This PR addresses the issue described above.

CHANGELOG.md entry:

[bugfix] whitespace fix whitespace "check-module" to properly lint and fix errors (#2401)

[palantirtech]

Thanks for your interest in palantir/tslint, @canufeel! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[andy-hanson]

Well, it should be. If you have:

import {
  a,
  b,
} from "foo";

I would expect the newline between b, and } to count as whitespace.

[canufeel]

😄 Let's face the facts. Newline is newline. Whitespace is whitespace. If newline counts as a whitespace should we treat it as a trailing whitespace as well and trigger the error here?

I think there is a room for new rule here - something like treat-newline-as-whitespace but otherwise i would really think twice by making newline to be treated as whitespace.

[adidahiya]

Newline is newline. Whitespace is whitespace.

I don't think it's so black and white. For the purposes of the whitespace rule where you want to see a space after { and before } in a single-line import statement, the rule should consider newlines in multiline import statements as whitespace. I still expect the following to produce failures:

import {a,
        ~
  b} from "foo";
   ~

but @andy-hanson's example should be valid.

Being pedantic about the semantics of what a "whitespace" is vs. what a "newline" is doesn't help us here; what matters most is the intent of the lint rule and the formatting or style consistency it helps us achieve.

[adidahiya]

mostly looks good! I have the same concerns as andy though.

[adidahiya]

Newline is newline. Whitespace is whitespace.

I don't think it's so black and white. For the purposes of the whitespace rule where you want to see a space after { and before } in a single-line import statement, the rule should consider newlines in multiline import statements as whitespace. I still expect the following to produce failures:

import {a,
        ~
  b} from "foo";
   ~

but @andy-hanson's example should be valid.

Being pedantic about the semantics of what a "whitespace" is vs. what a "newline" is doesn't help us here; what matters most is the intent of the lint rule and the formatting or style consistency it helps us achieve.

[adidahiya]

friendly ping @canufeel

[canufeel]

@adidahiya Hey hi! Would try to find some time and work on it tomorrow.

[canufeel]

@adidahiya @andy-hanson Hey hi! You might want to have a look at it now. Described concerns seems to have been addressed. Let me know 😉

[adidahiya]

nice, thanks @canufeel!