-
Notifications
You must be signed in to change notification settings - Fork 889
Allow source imports to be evaluated similar to named imports #1466
Conversation
optionExamples: ["true", '[true, {"named-imports-order": "lowercase-first"}]'], | ||
optionExamples: [ | ||
"true", | ||
'[true, {"named-imports-order": "lowercase-first", "source-imports-order": "lowercase-last"}]', |
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.
is it actually useful to set a different option for the import names vs. the import sources? I guess if we wanted to consolidate those options into one, it might be a breaking change... but this feels a bit ugly
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.
My rationale was:
- Breaking change if options are consolidated (renamed).
- The initial implementation is actually 'lowercase-last' for source-imports and 'case-insensitive' for named-imports, which seems to indicate a use-case envisioned, at least, by the author.
I don't mind changing if you guys think that we should only have one option though.
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.
what do you think @danvk?
for now I think having these separately configurable is probably fine; we'll impose a sane default
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.
I'd be fine with separate options. Module names tend to be more uniform (mostly lowercase, hyphenated) so I doubt this setting will matter much either way.
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.
@danvk that's true for external dependencies, but much less true for relative path imports of local modules
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.
although it's good form to name all local files in that way as well to avoid filesystem case issues and the like,
Other than naming, looks good @markwongsk! Happy to merge it after that's tweaked |
Sweet, thanks @markwongsk! |
This fixes #1431