Development registrar client #4
Conversation
SalmaElsoly
force-pushed
the
development_registrar_client
branch
from
0f24159 to
a546fa6
Compare
There was a problem hiding this comment.
Great work ya Salma 👏
I have some suggestions :
- I think we are going to use the module types in other places in the project rather than the module itself; IMO it's better to have an outer directory for types
- we need some extra info about how to start the project what are the requirements how to build and how to run tests.
That was an overview of the code will test the functionality and comment back with the results.
| "type": "git", | ||
| "url": "git+https://github.com/threefoldtech/tfgridv4-sdk-ts.git" | ||
| }, | ||
| "scripts": { |
There was a problem hiding this comment.
It will be great if we have some example scripts, like we have in https://github.com/threefoldtech/tfgrid-sdk-ts/tree/development/packages/grid_client/scripts
There was a problem hiding this comment.
I think we need to wrap the requests with try-catch blocks
| throw new Error("TwinID is not found"); | ||
| } | ||
| const farmName = farm.farm_name; | ||
| if (!farmName || farmName.length < 1 || farmName.length > 40) { |
There was a problem hiding this comment.
Create a const variable at the beginning
const MAX_FARM_NAME_LENGTHthis will enhance the readability and maintenance of the code;
if we need to increase that number later we do not need to search for where do we use it; and we may change it here but forget to change it in the error message or any other place that we may use it
| let twinID = 1; | ||
| let farmID = 1; | ||
|
|
||
| test("create farm", async () => { |
There was a problem hiding this comment.
I think we can add some more test cases:
- create a farm with no existing twin ID
- create a farm with an already existing farm name/id
- create a farm with an invalid name
| this.client = client; | ||
| } | ||
|
|
||
| async registerNode(node: NodeRegistrationRequest): Promise<NodeRegistrationResponse> { |
There was a problem hiding this comment.
Should we add any validation to the node data? Not sure if it's correct to rely on the server validation
There was a problem hiding this comment.
here as well I think we need more test cases; invalid node data, invalid farm id etc
There was a problem hiding this comment.
can we add unit tests for those functions?
| // "disableReferencedProjectLoad": true, /* Reduce the number of projects loaded automatically by TypeScript. */ | ||
|
|
||
| /* Language and Environment */ | ||
| "target": "es2016" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */, |
There was a problem hiding this comment.
is there a reason behind using es2016 if not I suggest using the latest
There was a problem hiding this comment.
and removing the commented lines will be much better
Description
implemented client for registrar :
Setup monorepo
used yarn with lerna + prettier + eslint
Related issues
#2 (comment)
#1 (comment)