fix: `import-name` options bugs and documentation by dosentmatter · Pull Request #882 · microsoft/tslint-microsoft-contrib

dosentmatter · 2019-07-19T05:30:34Z

PR checklist

Addresses an existing issue: fixes #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

import-name rule has an off-by-one bug where it selects options starting from the wrong index. I updated the tests to match this.

I also noticed that for camelCase, it allowed the first letter to be upper-case. any-case is either PascalCase or camelCase, but since camelCase allowed the first letter to be upper case, any-case and camelCase are currently the same. I enforced the first letter to be lowercase.

I also updated the documentation. It's a bit lacking right now. It doesn't explain all of the allowed options.

Is there anything you'd like reviewers to focus on?

Not completely related but, I found this bug from #881. In #881, I had other problems.

msftclas · 2019-07-19T05:30:47Z

All CLA requirements met.

dosentmatter · 2019-07-19T07:26:13Z

Wonder why Travis CI is failing lint for node 6. The previous build failed too. strict-comparisons is a new rule. I see a fix for the false positive. Perhaps that will fix it. Haven't looked at why it only fails on node 6 and not other versions.

IllusionMH · 2019-07-19T07:32:29Z

@dosentmatter thank you for the interest and PR. Will check it on weekend.

As for Node 6 - it was shipped with npm v3 that doesn't support package-lock.json files. Therefore it installs latest version of TSLint that has false positives instead of fixed version like builds with Node 8+. Error will go away with next TSLint release.

The options begin at index 0. The previous developer might have gotten it mixed up with initial `true` in the config array which controls `ruleSeverity`, not `ruleArguments`.

Add documentation and examples of all the `import-name` options.

dosentmatter · 2019-07-21T17:20:59Z

Actually, I made a mistake. Currently, any-case and camelCase are not exactly the same.

The current functionality is the following:
`PascalCase' allows (first letter must be uppercase):

import Foo from './foo';
import Foo from './Foo';

camelCase allows (first letter must match file name casing):

import foo from './foo';
import Foo from './foo';

any-case' allows (first letter is any case - PascalCase|camelCase`):

import foo from './foo';
import Foo from './foo';
import foo from './Foo';
import Foo from './foo';

What this means is that camelCase currently enforces that the first letter matches the file name casing. I updated my PR commit message. Let me know the desired functionality. If this wiki on camelCase is correct, it says

Some people and organizations, notably Microsoft,2 use the term camel case only for lower camel case

`camelCase` currently allows the first letter to be uppercase by enforcing that it matches the casing of the file name first letter. Enforce the first letter to be lowercase.

IllusionMH

Overall changes to docs and code (except now redundant update to makeCamelCase) looks good, however this will be breaking change and might require better approach.

IllusionMH · 2019-07-21T23:59:53Z

README.md

      <td>
        The name of the imported module must match the name of the thing being imported. For special characters (<code>-</code>, <code>.</code>, <code>_</code>) remove them and make the following character uppercase.
-        For example, it is valid to name imported modules the same as the module name: <code>import Service = require('x/y/z/Service')</code> and <code>import Service from 'x/y/z/Service'</code>.
+        For example, it is valid to name imported modules the same as the module name: <code>import Service = require('./x/y/z/Service')</code> and <code>import Service from './x/y/z/Service'</code>.


Is this change because of defaults?

IllusionMH · 2019-07-22T00:01:11Z

README.md

        Since version 2.0.9 it is possible to configure this rule with a list of exceptions.
-        For example, to allow <code>underscore</code> to be imported as <code>_</code>, add this configuration: <code>'import-name': [ true, { 'underscore': '_' }]</code>
+        The default is to ignore this rule for external modules <code>ignoreExternalModule: true</code> and to use camelCase <code>case: 'camelCase'</code>.
+        To not ignore this rule for external modules, add this configuration: <code>'import-name': [true, null, null, { ignoreExternalModule: false }]</code>.


Suggested change

To not ignore this rule for external modules, add this configuration: <code>'import-name': [true, null, null, { ignoreExternalModule: false }]</code>.

To apply this rule for external modules, add this configuration: <code>'import-name': [true, null, null, { ignoreExternalModule: false }]</code>.

IllusionMH · 2019-07-22T00:02:24Z

src/importNameRule.ts


    function makeCamelCase(input: string): string {
-        return input.replace(
+        let result = `${input.charAt(0)}${input.slice(1)}`;


Doesn't look like this changes anything (without .toLowerCase())

IllusionMH · 2019-07-22T00:03:05Z

package-lock.json

          "version": "2.1.1",
          "bundled": true,
-          "dev": true,
-          "optional": true


Could you please rollback these changes to package-lock.json?

IllusionMH · 2019-07-22T00:05:54Z

src/importNameRule.ts

        if (options.ruleArguments instanceof Array) {
            options.ruleArguments.forEach((opt: unknown, index: number) => {
-                if (index === 1 && isObject(opt)) {
+                if (index === 0 && isObject(opt)) {


This will be a breaking change. I thought that this might be side effect of migration to new rule format, however this behavior was in 6.0.0.
Looks like better solution is needed to avoid breaking change for those who added one more element to options array to match expected indices

IllusionMH · 2019-07-22T00:17:55Z

I think that it might be possible to avoid breaking change with checks of structure check arguments length and then types arguments e.g. options.ruleArguments[0] is object, options.ruleArguments[1] is array, options.ruleArguments[2] is object (or null or undefined for elements that were skipped).
If not matched - check with shift by one element (to match current behavior in master).

@dosentmatter @JoshuaKGoldberg what do you think on about this approach? Is it possible to do it in general case? Is it worth to implement or better to consider earlier 7.0 with small scope?

dosentmatter · 2019-07-22T02:38:15Z

Hey @IllusionMH, thanks for the review. I'll get to them when I have time. But I just want to respond to one of your comments first before I lose my train of thought.

I think that it might be possible to avoid breaking change with checks of structure check arguments length and then types arguments e.g. options.ruleArguments[0] is object, options.ruleArguments[1] is array, options.ruleArguments[2] is object (or null or undefined for elements that were skipped).
If not matched - check with shift by one element (to match current behavior in master).

@dosentmatter @JoshuaKGoldberg what do you think on about this approach? Is it possible to do it in general case? Is it worth to implement or better to consider earlier 7.0 with small scope?

I don't think it is possible to retain complete compatibility with the old code. The old code allowed element 0 to be any type, not just falsy values. It was completely ignored. So let's go over the case with a s length 1 options array:

[{}]
[true]
[undefined]
[null]
[{ 'React': 'react' }]
[{ 'Foo': './foo' }]

Although it is unlikely for someone to have an options array like this, all of these used to be ignored. If we now go and pass it to this.extractReplacements() because it is an object, it might break someone's linting. I looked at the code and ignoreExternalModule doesn't override the replacements, so React and Foo will be enforced.

However, maybe we can assume that not many people knew how to skip the 0th element. The base tslint.json just used true. The docs didn't explain it - are we required to maintain backward compatibility with an undocumented API? All the docs said was that you can pass in replacements to the 0th element, but it didn't work anyway because it was ignored. Searching up issues, I haven't seen anyone complain until #881, which was 15 days ago. In #881, I explained how to use the element skip workaround.

The only way you would know for sure to skip the 0th element is if you read the code, which is how I found it out.

JoshuaKGoldberg · 2019-12-15T04:34:15Z

Closing this PR per #876. It's always sad to see good work being closed, but we don't have the staffing or relevancy to take on more features. Thanks everyone!

dosentmatter force-pushed the master branch from 6bda5d1 to d91420d Compare July 19, 2019 05:33

dosentmatter changed the title ~~Fix import-name options bugs and documentation~~ Fix: import-name options bugs and documentation Jul 19, 2019

dosentmatter changed the title ~~Fix: import-name options bugs and documentation~~ fix: import-name options bugs and documentation Jul 19, 2019

IllusionMH self-requested a review July 19, 2019 07:32

IllusionMH added the PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! label Jul 19, 2019

dosentmatter added 2 commits July 19, 2019 00:41

Fix import-name options indexing off-by-one

090be13

The options begin at index 0. The previous developer might have gotten it mixed up with initial `true` in the config array which controls `ruleSeverity`, not `ruleArguments`.

Update import-name README.md

b26592e

Add documentation and examples of all the `import-name` options.

dosentmatter force-pushed the master branch from d91420d to 971f8b5 Compare July 19, 2019 07:44

dosentmatter mentioned this pull request Jul 20, 2019

import-name rule renaming map is not working #881

Closed

Make import-name camelCase begin with lowercase

5e29832

`camelCase` currently allows the first letter to be uppercase by enforcing that it matches the casing of the file name first letter. Enforce the first letter to be lowercase.

dosentmatter force-pushed the master branch from 971f8b5 to 5e29832 Compare July 21, 2019 17:24

IllusionMH suggested changes Jul 22, 2019

View reviewed changes

IllusionMH added Status: In Discussion Please continue discussing the proposed change before sending a pull request. Type: Breaking Change and removed PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! labels Jul 22, 2019

JoshuaKGoldberg closed this Dec 15, 2019

	To not ignore this rule for external modules, add this configuration: <code>'import-name': [true, null, null, { ignoreExternalModule: false }]</code>.
	To apply this rule for external modules, add this configuration: <code>'import-name': [true, null, null, { ignoreExternalModule: false }]</code>.

Conversation

dosentmatter commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

Uh oh!

msftclas commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dosentmatter commented Jul 19, 2019

Uh oh!

IllusionMH commented Jul 19, 2019

Uh oh!

dosentmatter commented Jul 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IllusionMH left a comment

Choose a reason for hiding this comment

Uh oh!

IllusionMH Jul 21, 2019

Choose a reason for hiding this comment

Uh oh!

IllusionMH Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

IllusionMH Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

IllusionMH Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

IllusionMH Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

IllusionMH commented Jul 22, 2019

Uh oh!

dosentmatter commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshuaKGoldberg commented Dec 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dosentmatter commented Jul 19, 2019 •

edited

Loading

msftclas commented Jul 19, 2019 •

edited

Loading

dosentmatter commented Jul 21, 2019 •

edited

Loading

dosentmatter commented Jul 22, 2019 •

edited

Loading