Allow to list all registered decoders by the plugin #114
Conversation
|
Actually it would be better to have a |
|
|
||
| ```js | ||
| { | ||
| "controller": "device-manager/device", |
There was a problem hiding this comment.
Should rather be in a dedicated decoder controller
| "node": "knode-shaggy-salmon-94717", | ||
| "requestId": "e74e2d34-6699-4dbf-8f38-874c6d2b9e1a", | ||
| "result": { | ||
| "size": 2, |
There was a problem hiding this comment.
Useless property, if we want the array size we can just read the length property (or it's equivalent in another language)
lib/core-classes/DeviceService.ts
Outdated
| @@ -36,6 +36,13 @@ export class DeviceService { | |||
| this.decoders = decoders; | |||
| } | |||
|
|
|||
| async listDecoders() { | |||
There was a problem hiding this comment.
It would be interesting to return the name of each decoder and info about this decoder. For now I can only think about the kind of measures decoded by the decoder. In the future we may add other information like mandatory payload properties, number of decoded payloads, ...
lib/controllers/DeviceController.ts
Outdated
| * List all available decoders | ||
| */ | ||
| async listDecoders () { | ||
| return this.deviceService.listDecoders(); |
There was a problem hiding this comment.
Nitpicking: IMHO it's better to return a variable and then return it in the controller action. The purpose of this is to be able to see the return of an API action directly by seeing the code of the method
|
Can you change the PR title to have a nice changelog also please? |
rolljee
changed the title
|
@Aschen I took the shortcut in order to see if we needed a dedicated controller for this purpose, i'll glady move the code 👍 |
|
Also, this PR is missing appropriate labels |
| actions: { | ||
| list: { | ||
| handler: this.list.bind(this), | ||
| http: [{ verb: 'get', path: 'device-manager/decoders/_list' }] |
There was a problem hiding this comment.
In REST spec a get to the name of the resource in plural form mean to list elements
| http: [{ verb: 'get', path: 'device-manager/decoders/_list' }] | |
| http: [{ verb: 'get', path: 'device-manager/decoders' }] |
| @@ -63,8 +69,9 @@ export abstract class Decoder { | |||
| /** | |||
| * @param deviceModel Device model for this decoder | |||
There was a problem hiding this comment.
Comments should be updated as well
There was a problem hiding this comment.
but this hasn't changed 🤔
There was a problem hiding this comment.
I was talking about the constructor now takes 2 params instead of one
There was a problem hiding this comment.
Oh yes, i didn't understud it that way ! i'll change that with the last Nitpiking you added
lib/core-classes/Decoder.ts
Outdated
| throw new PreconditionError(`Missing property "${path}" in payload`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static serialize(decoders: Map<string, Decoder>): DecoderConstructor[] { |
There was a problem hiding this comment.
The serialize method should rather be an instance method, meaning the Decoder is responsible of it's own serialization decoder.serialize().
The DecoderService.list method should then call this method for every decoder
lib/types/DecodersContent.ts
Outdated
| @@ -0,0 +1,4 @@ | |||
| export interface DecoderConstructor { | |||
There was a problem hiding this comment.
Should be DecoderContent instead (like the file name which is good since it represents the serialized content of a decoder)
lib/core-classes/DecodersService.ts
Outdated
|
|
||
| async list(): Promise<DecoderContent[]> { | ||
| const decoders = Array | ||
| .from(this.decoders.keys()) |
There was a problem hiding this comment.
Here you could use this.decoders.values() to directly access the values
This Pull Request add the support of a new action In the controllerdevice-manager/devicescalledlistDecodersAdded a new controller called
DecodersControllerwith his dedicatedDecodersServicewith a new action calledlistThis new action allow the user the print out all currently registered decoders by the plugin
Close #114