NEW (Extension) @W-16096256@ Send class content as request to ApexGuru and get requestId back #114
Conversation
…u and get requestId back
| @@ -0,0 +1,173 @@ | |||
| /* | |||
There was a problem hiding this comment.
Increasing the code coverage % by adding these new tests to some unrelated code to this change.
…u and get requestId back - part2
| // This could throw an error for a variety of reasons. The API endpoint has not been deployed to the instance, org has no perms, timeouts etc,. | ||
| // In all of these scenarios, we return false. | ||
| const errMsg = e instanceof Error ? e.message : e as string; | ||
| outputChannel.error('***ApexGuru perm check failed with error:***' + errMsg); |
There was a problem hiding this comment.
Ideally we should move all these messages eventually to a message catalog. A super lightweight version that I created for the 5.x world is https://github.com/forcedotcom/code-analyzer-core/blob/dev/packages/code-analyzer-engine-api/src/messages.ts#L18 and an example of how it is used: https://github.com/forcedotcom/code-analyzer-core/blob/dev/packages/code-analyzer-eslint-engine/src/messages.ts and https://github.com/forcedotcom/code-analyzer-core/blob/dev/packages/code-analyzer-eslint-engine/src/engine.ts#L117
There was a problem hiding this comment.
Agreed! I would like to follow the pattern that IDX team follows that helps with i18n as well. I am not doing it as part of this. But definitely worth doing it when the 5.x integration work happens in the extension.
src/types.d.ts
Outdated
| export type ApexGuruInitialResponse = { | ||
| status: string; | ||
| requestId: string; | ||
| message: string; | ||
| } | ||
|
|
||
| export type ApexGuruQueryResponse = { | ||
| status: string; | ||
| message: string; | ||
| report: string; | ||
| } | ||
|
|
||
| export type ApexGuruProperty = { | ||
| name: string; | ||
| value: string; | ||
| }; | ||
|
|
||
| export type ApexGuruReport = { | ||
| id: string; | ||
| type: string; | ||
| value: string; | ||
| properties: ApexGuruProperty[]; | ||
| } |
There was a problem hiding this comment.
Why are all these types in a d.ts file? Adding a d.ts file is only needed if you are interacting with a javascript library that doesn't have its types exported for use in typescript.
Since you are already in typescript land, the tsconfig file can generate the d.ts files automatically if you wanted to export these types to be used by other typescript modules.
There was a problem hiding this comment.
I have moved the new types from d.ts file. I will create a separate PR right after this to get rid of d.ts file altogether.
| const response: ApexGuruInitialResponse = await connection.request({ | ||
| method: 'POST', | ||
| url: Constants.APEX_GURU_REQUEST, | ||
| body: JSON.stringify({ | ||
| classContent: base64EncodedContent | ||
| }) | ||
| }); |
There was a problem hiding this comment.
It looks like you are assuming that the response will always be of type ApexGuruInitialResponse. But is it possible that you do not get a response back that has a status field?
Besides the fact that this request method is template, the other reason why you aren't getting a compiler error here from typescript is because you put this type in a d.ts file which is unchecked when compiled.
I suggest you move your ApexGuruInitialResponse and other types into typescript officially (in a .ts - not a .d.ts file) to be compiled, then this will complain unless you cast with "as ApexGuruInitialResponse" to the request output.
But do we really want to do this casting? I would guess that you would want to first check the status code of the response. But now I see that the request method is templated so it is inferring the output type.
Do we know if the response object universally as something like a response code that can be used? Typically when making http requests, we validate the response to see if it is a 2** or a 4** or a 5** before we try to parse the response body.
There was a problem hiding this comment.
I moved the ApexGuruInitialResponse out out of the d.ts file. I noticed we don't get 2**/4** responses as part of the response. I'll bring this up with the ApexGuru team tomorrow in the meeting. I hope it should be easy fix for them. Btw, the connection request also does not give the status codes. So, I'll have to leave this as is for now.
src/apexguru/apex-guru-service.ts
Outdated
| } | ||
| } | ||
|
|
||
| export async function initiateApexGuruRequest(selection: vscode.Uri, outputChannel: vscode.LogOutputChannel) { |
There was a problem hiding this comment.
Can you add : string to end of this function signature to make obvious its return type.
src/apexguru/apex-guru-service.ts
Outdated
|
|
||
| if (response.status != 'new' && response.status != 'success') { | ||
| outputChannel.warn('***Apex Guru returned unexpected response:***' + response.status); | ||
| return ''; |
There was a problem hiding this comment.
I'm guessing we'll be adding in polling soon (in a future PR) in order to wait for the job to finish so we can get the results back and process the output. So my question is, is returning an empty string really what we want when the initiate request doesn't succeed. I would think we would just error and not return in order to prevent polling from even starting.
There was a problem hiding this comment.
A lot of this is TBD and so I just returned an empty string. Sure, I went ahead and made it return an error instead.
…u and get requestId back - part3
No description provided.