dxdc / homebridge-blinds

:sunrise: Homebridge Plugin to control my blinds over HTTP
https://www.npmjs.com/package/homebridge-blinds
ISC License
54 stars 26 forks source link

Organize the settings layout #44

Closed slavikme closed 4 years ago

slavikme commented 4 years ago

This PR resolves to the issue #42

dxdc commented 4 years ago

Awesome work @slavikme ! This is looking fantastic. I need to add the following missing options also. I think this is all of them.

And, does http_method that you've added for GET, POST, PUT also support the more advanced object configuration? E.g.,

      "http_method": {
        "body": "{}",
        "headers": {
          "API-Token": "aaabbbcccddd"
        },
        "method": "PUT"
      },
slavikme commented 4 years ago

I need to add the following missing options also. I think this is all of them.

  • unique_serial
  • webhook_http_auth_pass
  • webhook_http_auth_user
  • webhook_https
  • webhook_https_certfile
  • webhook_https_keyfile
  • webhook_port

I think you should add them after the merge, as you can describe them better than me.


And, does http_method that you've added for GET, POST, PUT also support the more advanced object configuration? E.g.,

      "http_method": {
        "body": "{}",
        "headers": {
          "API-Token": "aaabbbcccddd"
        },
        "method": "PUT"
      },

Yes, I followed their documentation page and I tried to preserve the original structure. But, http_method is not a good name for HTTP request properties, It is confusing. Maybe http_props or http_options will be more suited.

slavikme commented 4 years ago

Also, don't forget to assign the issue #42 to this PR.

dxdc commented 4 years ago

@slavikme thanks for all of your work here. I hadn't worked much with this structure before, so your changes really helped set the foundation for the changes here: 7c4645a

(Btw, in your version there were some minor issues, like success_codes and http_method didn't work)

There are a number of changes:

I welcome any testing and further comments you're able to provide. A few additional notes:

  1. http_method is a historic part of this plugin (before I was the maintainer), and agree it is confusing. It supports both a string object (e.g., PUT) or a request object with more advanced parameters.

However, each url (e.g., up_url) is also capable of being a request object. This was done to allow users to have full flexibility over each request... but it's also convenient to allow general settings if the parameters for each request are identical. Ultimately I would like to deprecate some parameters, but it may introduce a breaking change depending on how I do it.

  1. I cannot figure out if it's possible to adapt the JSON schema to allow for the possibility of adding unlimited headers, for example, so I've just hard coded them. It would be great to also allow additional properties in the request object... and also... ability to fully customize an up_url request object for example.
dxdc commented 4 years ago

http_method is a historic part of this plugin (before I was the maintainer), and agree it is confusing.

Deprecated: 2d9f20c

slavikme commented 4 years ago

Looks good. Awesome work! Didn’t know that conditional view is even possible.

slavikme commented 4 years ago
  1. I cannot figure out if it's possible to adapt the JSON schema to allow for the possibility of adding unlimited headers, for example, so I've just hard coded them. It would be great to also allow additional properties in the request object... and also... ability to fully customize an up_url request object for example.

A better approach would be to provide the structure an array of objects (with key and value fields) for the header section. Don’t worry about the uniqueness of keys, they shouldn’t be unique (eg. set-cookie can appear more than once). This change affects the code, which needs to be changed accordingly.

dxdc commented 4 years ago

A better approach would be to provide the structure an array of objects (with key and value fields) for the header section. Don’t worry about the uniqueness of keys, they shouldn’t be unique (eg. set-cookie can appear more than once). This change affects the code, which needs to be changed accordingly.

Well, this is how request does it, and I just pass the same object: https://github.com/request/request#custom-http-headers

Also, users can add any other compatible parameters this way :) Not sure if there's a simple solution. I wanted to allow users to add dynamic keys but maybe for that they will have to tap into the JSON .. lol. Let me know if you have any changes otherwise I can merge!

slavikme commented 4 years ago

Nothing special. You can merge. Thanks.

slavikme commented 4 years ago

@dxdc Are you gonna bump to version 1.3.22?

dxdc commented 4 years ago

@slavikme I can.. was also thinking about #45