-
Notifications
You must be signed in to change notification settings - Fork 889
Add option for "allow-namespace-imports" to noDuplicateImports rule #4524
Conversation
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. |
3612884
to
d4d1185
Compare
There was a problem hiding this 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.
d4d1185
to
02a8056
Compare
Thanks for the feedback. I believe I have addressed all your issues, but for brevity, here are my comments on your comments.
Thanks for your help!! |
Looks like all the tests passed this time 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, thanks @EdwardDrapkin! 💖
Yes, the changelog entry is a bit too long haha. I'll go ahead and edit it. |
Is there a reason why this option isn't the default? |
@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? |
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. |
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). |
- 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!
- 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!
- 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!
* 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
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:
With the option set, the following error is still caught:
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 tono-duplicate-imports
rule