Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add option for "allow-namespace-imports" to noDuplicateImports rule #4524

Merged
merged 1 commit into from Feb 25, 2019

Conversation

EdwardDrapkin
Copy link
Contributor

@EdwardDrapkin EdwardDrapkin commented Feb 19, 2019

PR checklist

Overview of change:

Adds an option "allow-namespace-imports" that can be passed to the no duplicate imports rule that will make it skip over lines containing namespace imports.

This option will fix the following fairly common pattern that currently errors the rule:

import * as React from 'react';
import { NamedExoticComponent } from 'react';

With the option set, the following error is still caught:

import * as React from 'react';
import { NamedExoticComponent } from 'react';
import { ExoticComponent } from 'react';

When the new option is enabled, wildcard imports will be allowed on a separate line from named imports, but the rule will still fail for multiple lines of named imports as well as multiple wildcard imports from the same module. To enable the new behavior pass the following object as an option to the rule in your tslint.json: { "allow-namespace-imports": true }

CHANGELOG.md entry:

[new-rule-option] added allow-namespace-imports option to no-duplicate-imports rule

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @EdwardDrapkin! 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.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks great! Nice clean change. 😄 Just some simplification internally and future-proofing of the options.

It looks like the lint build task is failing because of a couple of Prettier complaints; you'll want to fix those up too.

src/rules/noDuplicateImportsRule.ts Outdated Show resolved Hide resolved
src/rules/noDuplicateImportsRule.ts Outdated Show resolved Hide resolved
@EdwardDrapkin
Copy link
Contributor Author

EdwardDrapkin commented Feb 25, 2019

Thanks for the feedback.

I believe I have addressed all your issues, but for brevity, here are my comments on your comments.

  1. I updated the call to pass options to ctx per your suggestion. Didn't know this was possible, thanks for the help.

  2. I figured out how to write tests and added a test case for this option because the option is a bit more complex. I believe I covered all of the possible permutations of error/not error when the rule is enabled.

  3. I updated the rule options to be an object with the format { "allow-namespace-imports": boolean }.

  4. The rule now tracks named imports separately from namespace imports. The benefit is that now multiple wildcard imports will error out alongside multiple named imports. You also get a separate, special error message specifically for multiple wildcard imports.

  5. I updated the changelog entry suggestion, but I fear I may have taken it from too short to too long.

Thanks for your help!!

@EdwardDrapkin
Copy link
Contributor Author

EdwardDrapkin commented Feb 25, 2019

Looks like all the tests passed this time 🎉

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, thanks @EdwardDrapkin! 💖

@JoshuaKGoldberg
Copy link
Contributor

Yes, the changelog entry is a bit too long haha. I'll go ahead and edit it.

@huyz
Copy link

huyz commented Feb 25, 2019

Is there a reason why this option isn't the default?

@JoshuaKGoldberg
Copy link
Contributor

@huyz it happened not to be in this PR, but that's a good idea. Perhaps it should - would you mind filing a new issue?

@EdwardDrapkin
Copy link
Contributor Author

Is there a reason why this option isn't the default?

I didn't want to presume to change default behavior is the only reasoning I had to make it opt-in instead of opt-out.

@naingaungphyo
Copy link

naingaungphyo commented Oct 31, 2019

Is there any reason or advantages for not using React.NamedExoticComponent and React.ExoticComponent, like we use React.Component only with one import statement(1st line).

peterblazejewicz added a commit to peterblazejewicz/DefinitelyTyped that referenced this pull request Mar 8, 2020
- constants import changed
- re-export redefined
- contants module reshape to comply with native module details.

Using namespace import and named import for constant required TSLint
config update to allow named imports:
see: palantir/tslint#4524

/cc @43081j

Thanks!
peterblazejewicz added a commit to peterblazejewicz/DefinitelyTyped that referenced this pull request Mar 11, 2020
- constants import changed
- re-export redefined
- contants module reshape to comply with native module details.

Using namespace import and named import for constant required TSLint
config update to allow named imports:
see: palantir/tslint#4524

/cc @43081j

Thanks!
peterblazejewicz added a commit to peterblazejewicz/DefinitelyTyped that referenced this pull request Mar 26, 2020
- constants import changed
- re-export redefined
- contants module reshape to comply with native module details.

Using namespace import and named import for constant required TSLint
config update to allow named imports:
see: palantir/tslint#4524

/cc @43081j

Thanks!
uniqueiniquity pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Mar 30, 2020
* feat(karma): update constants export details

- move Constatns to separate file
- export as named re-export from main module
- add missing type for 'PORT' - should be `number` OR `string` depending
on the source of this contant. If read from ENV it will be a string
always.
- add missing documentation
- move LOG types to use string literal types to use in the values in
typechecks and intelllisense
- tests updated

https://github.com/karma-runner/karma/blob/master/lib/constants.js

Thanks!

* Resolve pull request comments:

- constants import changed
- re-export redefined
- contants module reshape to comply with native module details.

Using namespace import and named import for constant required TSLint
config update to allow named imports:
see: palantir/tslint#4524

/cc @43081j

Thanks!

* Refine import details as per PR comment

/cc @43081j
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants