Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add devicetype.getById and getByType (with correct validator) #93

Closed
wants to merge 5 commits into from
Closed

Conversation

Jean-PhilippeD
Copy link
Contributor

No description provided.

@Pierre-Gilles
Copy link
Contributor

Thx for your PR !

A few feedbacks :

  • Try to be consistent with the rest of Gladys. For example, you create a function deviceType.getById which takes as a parameter a number, the id of the deviceType. In Gladys, others getById functions takes an object containing options ( see gladys.user.getById for example ). This allows us to put others options in it, and when you use it it's more understandable. ( gladys.user.getById({id: 1}) is clearer --to my opinion-- ). The use of sqlUnique means you update a single row, so that you don't have to do a if(deviceTypes.length) return deviceTypes[0];.
  • Why are you returning 'null' when the array is empty in getByType? An API should returns consistent type, if it returns an array someday, it must always returns an array. If there is an error, it should throw an error, not return null.
  • Your comments are comments from another piece of code ( exemple in getByType : "// get all deviceTypes for a given tag", but we don't speak about tags here)
  • For your CONCAT(d.name, " - ", dt.type, " - ", r.name), I recently (today) added two new attributes in deviceType : identifier & name. So that now I will display the name instead of the type. We talked about that a few moments ago and the forum, and it was true that something was missing in the deviceType table. The type is more for Gladys to understand what type of deviceType it is, and the name is to display for the user. What do you think about it ? :) But I will keep the room you added, that's a good idea.
  • Finally, can you explain me again why this getByType is needed ? I don't get where it can be useful.

Thanks again for the PR, theses are juste general feedbacks to make Gladys code as strong, stable & consistent as possible, it's not to annoy you :) Keep in mind that these changes will affect all gladys users ( since the beginning, Gladys has been downloaded more than 12 000 times, that's a lot of people :D ), and these are changes to Gladys API, so it needs to be consistent.

@Jean-PhilippeD
Copy link
Contributor Author

In response to your points:

  • Ok I understand and you're right :) I let your fix the syntax
  • I'm going to verify that, you're right again, it's not really clean. I think I should return an empty array
  • I used a previous getByTag as I've spoken in the forum and I let error during the copy/paste
  • For this point, it's a piece of code I've changed few month ago and I let it ...
  • For my need, I want to get the state of all kind of device (door and windows sensor) when I'm leaving so I needed to get all devices by type.

@Jean-PhilippeD
Copy link
Contributor Author

In fact, I'll do the change and push you a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants