Align client classes with the TSP specification #4
Conversation
src/models/entry.ts
Outdated
| /** | ||
| * Indicate if the entry will have row data | ||
| */ | ||
| hasRowModel?: boolean; |
There was a problem hiding this comment.
update to generic name in TSP. I think it's hasData
|
It matches the master of the trace-server-protocol as of today. It will be a breaking change for the trace extension and will need a matching patch in the trace server. I'm working on it, once the builds are fixed there! |
| const url = this.baseUrl + '/experiments/' + expUUID + '/outputs/timeGraph/' + outputID + '/tree'; | ||
| return await RestClient.post<GenericResponse<EntryModel<M, H>>>(url, parameters); | ||
| return await RestClient.post<GenericResponse<EntryModel<TimeGraphEntry>>>(url, parameters); |
There was a problem hiding this comment.
@tahini You are removing the generic types here and below. I'm not sure about the implications. Does it mean that the server has to send a TimeGraphEntry in the response and can't send something like (MyEntry extends TimeGraphEntry)? It would be more strict, right?
There was a problem hiding this comment.
Indeed! It is my understanding from a protocol point of view that we should not send random fields that are not documented, but rather let the protocol provide a way to describe the potential arbitrary fields (like name, tooltip, datatype, description, etc), so that implementation can have one generic way to handle them. The consumers should not have to know about all the various entry types, so yeah, the server will have to send a TimeGraphEntry with extra fields described as labels. It would be more strict, but for a good cause I think.
That is the reason behind this issue here: eclipse-cdt-cloud/trace-server-protocol#35 Let me know what you think
Fixes eclipse-cdt-cloud#3 The rest methods should not use generic types, as it would imply that consumers of this client need to know about the details of each potential sub-class, which is one thing we should avoid. Rather the protocol should be self-descriptive to provide extra data in one specific way. Signed-off-by: Geneviève Bastien <[email protected]>
A few fields have been renamed. Signed-off-by: Geneviève Bastien <[email protected]>
The composite directive prevented yarn build from building in the lib directory. Signed-off-by: Geneviève Bastien <[email protected]>
|
Thanks for the review, I'll merge when all the components are ready to merge (the theia-trace-extension one will need to be updated once this is in to link to the new next version of this packages) |
Fixes #3
Signed-off-by: Geneviève Bastien [email protected]