Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Apr 20, 2018

  • Don't make a separate call per completion; provide lists of completions that will be included/excluded, or (preferably) a list of every completion.
  • Use an object with many optional properties, so we don't have to write e.g. verify.completionsListContains("x", undefined, undefined, undefined, undefined, undefined, { includeCompletionsForModuleExports: true });(This gets more annoying as we add more options.)
  • Support the location being an array of locations if the completions are expected to be the same at every location.
  • Assert that somethings like hasAction and sourceDisplay are undefined if not mentioned in the test.
    (Also discovered Duplicate completion entries for undefined #23587 and Completion details kind disagrees with entry kind #23594)

@ghost ghost requested review from armanio123 and sheetalkamat April 20, 2018 23:02
@ghost ghost force-pushed the verifyCompletions branch from 61b751c to feb1c62 Compare April 20, 2018 23:13
@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2018

👍 would like to get other's feeling about the API flow in general. pinging @sheetalkamat, @rbuckton, @sandersn, @RyanCavanaugh and @amcasey.

I also believe this should be faster running the tests, instead of requiring for the same completions multiple times.

@ghost
Copy link
Author

ghost commented Apr 20, 2018

There is a memoWrap function in fourslash.ts that attempts to reduce the cost of making repeated calls. Although it doesn't seem to apply to getCompletionsAtPosition.

@ghost ghost merged commit 9f4abe2 into master May 1, 2018
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Minor feedback.

readonly sourceDisplay?: string,
};

type Many<T> = T | ReadonlyArray<T>;
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I find it confusing that "Many" might be mean "one". I might have called it "OneOrMore".

Copy link
Author

Choose a reason for hiding this comment

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

It could be 0 though... this is just a slightly less verbose way of writing an array. How about ArrayOrSingle?

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I guess I'm not sure I understand why we're not just using an array.

Copy link
Author

Choose a reason for hiding this comment

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

In many places (such as at) there's usually only one element, so it saves a lot of typing. This type is usually converted using toArray.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't think about the call site. In Roslyn, we had something similar, but it was a way to avoid allocating the array, which didn't seem to be (and isn't) a concern here. Yeah, I like ArrayOrSingle.

export interface VerifyCompletionsOptions {
readonly at?: Many<string>;
readonly isNewIdentifierLocation?: boolean;
readonly are?: Many<ExpectedCompletionEntry>;
Copy link
Member

Choose a reason for hiding this comment

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

I find the name "at" a little unclear and the name "are" very unclear.

Copy link
Author

@ghost ghost May 2, 2018

Choose a reason for hiding this comment

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

at could be atMarker -- not sure what to replace are with -- it's what the completions are. areExactly? Keep in mind the name will appear thousands of times so we don't want it to get too long. How about exact?

Copy link
Member

Choose a reason for hiding this comment

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

I figured out what they meant by reading the tests, but I still find the names unclear. I would have expected something like location or marker for at and something like all or exact for are (or a flag indicating that includes is exhaustive).

Copy link
Author

Choose a reason for hiding this comment

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

marker and exact sound good.

return;
public verifyCompletions(options: FourSlashInterface.VerifyCompletionsOptions) {
if (options.at !== undefined) {
if (typeof options.at === "string") {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could be a helper function that operates on Many<T>?

this.verifyCompletionEntry(found, include);
}
}
else {
Copy link
Member

Choose a reason for hiding this comment

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

Are includes and excludes mutually exclusive?

Copy link
Author

Choose a reason for hiding this comment

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

No, good catch!

@ghost ghost mentioned this pull request May 15, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
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.

3 participants