Fix autocomplete suggestion #3554
Conversation
🦋 Changeset detectedLatest commit: 23d9616 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
awesome, thank you @rrousselGit! can you add some unit tests for the failing cases? |
|
Sure! But I'd like to know whether this is the right approach first. There are a few failing tests, and I'm not necessarily sure why. Do you have any clue on what could possibly be going wrong with this approach? |
| ) { | ||
| return; | ||
|
|
||
| /** Returns whether a token is before, at or after the cursor */ |
There was a problem hiding this comment.
| /** Returns whether a token is before, at or after the cursor */ | |
| /** Returns whether a token is before, at, or after the cursor */ |
| ); | ||
|
|
||
| if (compare === 'before') { | ||
| // The token is before the cursor, keep track of it |
There was a problem hiding this comment.
| // The token is before the cursor, keep track of it | |
| // The token is before the cursor; keep track of it |
| return; | ||
| } | ||
|
|
||
| // The token is at or after the cursor, we return the last token before the cursor |
There was a problem hiding this comment.
| // The token is at or after the cursor, we return the last token before the cursor | |
| // The token is at or after the cursor; we return the last token before the cursor |
|
@rrousselGit I will have to get back to you on that once I have time to build up a bit more context. Going into the parser takes great delicacy as you can see haha! I would suggest a TDD approach at first to learn how the parser works, and reading the tests, you can use yarn test --watch at the workspace root and it will run only the impacted/changed unit tests. in general, the tests for the language service are a TDD dream and relatively pure, though I plan to add integration tests to test broader spec expectations than are in the scope of unit tests, mostly to test integration with graphql library itself. So beware that the autocomplete tests are still relatively spec and context/edge case incomplete. Sometimes with unit tests i will test completion/etc at slightly different positions to try to catch edge cases but as you've found there are many! |
|
this is causing a lot of undesired regressions it seems. perhaps you can find another place to fix this issue? |
fixes #3553
I'll add tests later, assuming that those changes are the correct way to go. I'm not familiar enough with the codebase to know for sure :)