-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trace API: give indicator of response success status #3010
Trace API: give indicator of response success status #3010
Conversation
Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
|
||
private getResultLength(value: any): number | undefined { | ||
if (!value) { | ||
return 0; |
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.
should we return undefined
here?
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.
If the response is undefined, it indicates that there are no results and returning 0 is acceptable. For me, returning undefined
means that we have not yet determined whether the response contains valid results.
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.
Ok, it makes sense. Then we probably need to revisit line 106. If value = { someKey: "some value" }
, it now returns undefined, is that expected?
If 0 is expected in above case, shall we append ?? 0
like below?
return value?.length ?? value?.items?.length ?? 0;
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.
Then we probably need to revisit line 106. If value = { someKey: "some value" }, it now returns undefined, is that expected?
Yes. We can return undefined
for now. Maybe one day there will be a requirement to resolve some response values more accurately and we can improve it then.
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.
OK, LGTM then.
…r#3010) Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>
One use case for this API is to trace completion performance.
To support the feature Auto complete after new keyword #2010 ("new |"), we enabled completion trigger characters on whitespace. However, this also triggers many completion requests when typing whitespace for indentation. These requests are invalid and jdtls returns an empty array. We need a way to distinguish such noise data during tracing.