-
-
Notifications
You must be signed in to change notification settings - Fork 566
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
Supporting raw and pronto optional parameter without type specifier. #199
Comments
Are you sure this is a good idea? IMHO two variants (string without colons, string with colons and optional end) are straightforward. We shouldn't introduce more complexity here. Especially if you validate the optional parameters at home assistant properly you would be lost. At the moment you are able to implement a service schema which validates the optional parameters (repeats vs. frequency) dependending on a (required &) supplied command type. |
After giving this some more thought, I agree with that notion. It's better to keep the interface simple and try not to handle all weird corner cases. The error message will be just fine to notify the caller on that error. One thing that could be done to make it easier to spot the error would be echoing the command type which is considered invalid back, then there'd be no mistake whatsoever. What do you think? |
I agree with both of you, I did also have a concern about the optional parameter having different meanings. Either echo it back or return an error about not supporting it (If that is possible without it being as complex to support as different cases). |
I've actually omitted this case on purpose when implementing the auto-detection and prefixes. In addition to the reasons you've already mentioned (different meaning of the optional argument) there's also the fact that if the user needs to play with optional argument they should know what format they are dealing with. Adding few extra characters as a type prefix won't do any harm. I even think that explicit type declaration is still a lot better (even when no optional args are used) than using the auto-detection, as it makes things less likely to break. |
Ok, I think we are in agreement that this is better kept as it is, let's close this! |
I think that as we support auto detection of the command type without needing to specify the command type, we should also be able to use the optional parameter of those commands without needing to specify the command type.
I tried running the following two commands which both specify the optional parameter for each command type:
Here are the error messages:
The text was updated successfully, but these errors were encountered: