search

runestone2023 / Group-B

2 stars 0 forks source link

Added endpoint for driving the robot forwards and backwards #6

Closed Dan-cmyk closed 1 year ago

Dan-cmyk commented 1 year ago

Closes #1

eoleedi commented 1 year ago

Should validate the parameters sent to the /api/move/drive, and see if it is a valid value defined in DriveCom class. Also, it's not related to this PR, but having an auto-generated API document is good. #7

NamHoai1210 commented 1 year ago

Should validate the parameters sent to the /api/move/drive, and see if it is a valid value defined in DriveCom class. Also, it's not related to this PR, but having an auto-generated API document is good. #7

I suggest using Swagger for this... What do you think?

Dan-cmyk commented 1 year ago

Should validate the parameters sent to the /api/move/drive, and see if it is a valid value defined in DriveCom class.

As far as I know this can't be done with an Enum. If you can think of another representation of our data that can have methods, I'm all for it.

eoleedi commented 1 year ago

@Dan-cmyk I don't have previous experience with expressJS so I find it hard to suggest a new way to represent our data.

However, I am curious about the effect of "api/src/routes/InputValidation.ts". Isn't it for validation?

Also, I find a package that support a decorator @ISEnum. Maybe that's helpful? https://github.com/typestack/class-validator

NamHoai1210 commented 1 year ago

Should validate the parameters sent to the /api/move/drive, and see if it is a valid value defined in DriveCom class.

As far as I know this can't be done with an Enum. If you can think of another representation of our data that can have methods, I'm all for it.

I think Enum is ok, if there are no other special function while the robot moving... we can use Object.values<number>(DriveCom).includes(value); for validation

NamHoai1210 commented 1 year ago

But, i wonder why not merge driving robot turn left and right to the DriveCom type

Dan-cmyk commented 1 year ago

However, I am curious about the effect of "api/src/routes/InputValidation.ts". Isn't it for validation?

@eoleedi Is it not validation to check if the data we get is of the correct type? I did not have a good name for the function and its current name describes what it currently does, if you have a better name and/or more things it should do, please make a change request.

Dan-cmyk commented 1 year ago

But, i wonder why not merge driving robot turn left and right to the DriveCom type

@NamHoai1210 ¯_(ツ)_/¯ It is not part of the connected issue #1 so we might do that when we get to the relevant issue #2.

Dan-cmyk commented 1 year ago

Should validate the parameters sent to the /api/move/drive, and see if it is a valid value defined in DriveCom class.

As far as I know this can't be done with an Enum. If you can think of another representation of our data that can have methods, I'm all for it.

I think Enum is ok, if there are no other special function while the robot moving... we can use Object.values<number>(DriveCom).includes(value); for validation

If you prefer this way of validating instead of how I did it, could you make a Change request?

eoleedi commented 1 year ago

However, I am curious about the effect of "api/src/routes/InputValidation.ts". Isn't it for validation?

@eoleedi Is it not validation to check if the data we get is of the correct type? I did not have a good name for the function and its current name describes what it currently does, if you have anything better, please make a change request.

Sorry, I didn't quite understand the meaning of this. Isn't the data we get from API requests always plain JSON? In this case, how can a function validate if the received data is DriveCom?

I dig into the source code and consider that isDriveCom will return true no matter what the value is. So my question is what is the function validating? If I understand wrong, please correct me.

Dan-cmyk commented 1 year ago

However, I am curious about the effect of "api/src/routes/InputValidation.ts". Isn't it for validation?

@eoleedi Is it not validation to check if the data we get is of the correct type? I did not have a good name for the function and its current name describes what it currently does, if you have anything better, please make a change request.

Sorry, I didn't quite understand the meaning of this. Isn't the data we get from API requests always plain JSON? In this case, how can a function validate if the received data is DriveCom?

I dig into the source code and consider that isDriveCom will return true no matter what the value is. So my question is what is the function validating? If I understand wrong, please correct me.

Sorry if I have been unclear.

Yes we will get plain JSON, but in that JSON we expect a correct value to be sent, in this case 1, 0 or -1 depending on how the client wants the robot to move. In JS/TS a JSON object is treated as a dict so we need to get a value from a key and validate that this value is one we are expecting (1, 0, -1). This is done in "api/src/routes/api.ts" on row 22 validate(['com', isDriveCom]), that will check that the value from the key "com" is valid using the function isDriveCom.

isDriveCom is admittedly a kind of jankey function as it will "convert" the given value to DriveCom and see if it is indeed in the DriveCom enum. Because JS, that TS is compiled into, does not truly have a enum the conversion in this case will not do anything, it is just to keep TS happy. And as enums is not a part of JS they are really just a dict, so isDriveCom checks if a given value exists in a dict and will therefore not always return true.