Allow regex groups *after* keyword groups #335
Conversation
Many tests rely on 4-space indentation for code inserted by code actions, but if the tester has a different user setting, these tests fail because of the larger context. To avoid this implicit assumption, we need a workspace setting in the test workspace to guarantee indent size.
When import group settings had regex groups *after* keyword groups (such as 'Modules'), these regex groups would usually not be considered as the keyword group ahead of them would already coopt the relevant modules. This ensures that group assignment considers regex groups ahead of keyword groups, whilst retaining original group ordering for final import layout. All examples in the docs use regex groups *ahead of keywords*: in such situations, the result layout is unchanged with this commit. But we can now use regex groups *after* keywords, too 😉.
|
It's odd how I don't get the errors reported by CI on my side. But I did see the causes and pushed fixes, so it should end up working! Very curious why I get other failures locally. |
| export function importGroupSortForPrecedence(importGroups: ImportGroup[]): ImportGroup[] { | ||
| const regexGroups: ImportGroup[] = []; | ||
| const otherGroups: ImportGroup[] = []; | ||
| for (const ig of importGroups) { |
| @@ -0,0 +1,25 @@ | |||
| import * as chai from 'chai'; | |||
There was a problem hiding this comment.
please switch to an intend of 4 spaces (to adhere with the rest of the code)
(in the file)
There was a problem hiding this comment.
Dang! I did in the other PR (there's gonna be a conflict, btw, as this file is introduced in both, so once #336 is merged in, I'll re-work this so it merges fine).
|
Hey @tdd It's a pretty nice code you wrote, so it's all nice and square. Just one little comment to come bye since the rest of the code is written that way. Did you still experience other errors locally? I tried on my computer @ home and there I don't have any errors (anymore). |
|
A question on my side: Do you think this "feature" should be configurable? Or rather not? |
|
Hey @buehler, Config: nopeI don't think this requires configuration, as the ordering you'd use in Local errors: yup 😢As for errors: yes, I get them all the time. I wonder if this is due to a different TS version (yarn's lock promise being faulty, it put me with the latest TS 2.6, when the original code was 2.5). I tried disabling all my exts, still got issues (I have to disable Prettier and ESLint anyway when working on this, as my settings for them conflict with either coding or testing it). This is super-annoying, and I have no idea what gives. |
It was impossible to define regex groups when keyword groups were listed ahead of them and matching the same source. See #334 for details and examples. This addresses the issue.
Description
The code change itself is fairly minimal, adjusting the logic of
ImportManager#addImportsToGroups()to rely on a new utility function. Both have added tests that pass.A word about tests
Despite the badge saying
developpasses build, it failed on my machine right from the forked code. I get a timeout and 4 undefined import declarations. I don't know enough about TS' internals to figure out why this was, so I left these failing tests as-is, and just added my (passing) ones. No previously-OK test broke in the process, obviously 😉Also, there was an implicit assumption by many tests that indent size was 4 (VSCode's default, and the original author's). But anyone running tests with a different User Setting would see a lot of failures. I fixed this by forcing a 4-space indent in the test workspace's settings.
VSCode, TypeScript and me…
This is the first time ever I write TypeScript for actual production code (I'm more an ES.Next kinda guy) or write VSCode extension stuff, so I'm sorry if some of my code, test or otherwise, seems non-idiomatic: feel free to point me in the right direction for you. I tried to abide the original coding style (had to disable my ESLint/Prettier settings to avoid unduly reformatting everything), I hope it's good with you!
I do intend to contribute two other PRs to this ext, though: one to address a personal need that I apparently share with many (e.g. #316): an option that lets us sort imports by first specifier name, not by module name. And another purely for "regex grooming": I know JS regexes (and regexes in general) intimately, and it seems to me a lot can be improved on the way they're used by the ext; because this is not feature-related and quite subjective, I'll make a separate PR out of it.
Looking forward to your feedback! (and explanations about why these other tests failed, I'm curious).
Best,