8000 exposed content_type in rest_command by cmsimike · Pull Request #7101 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

exposed content_type in rest_command #7101

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Apr 18, 2017
Merged

exposed content_type in rest_command #7101

merged 9 commits into from
Apr 18, 2017

Conversation

cmsimike
Copy link
Contributor
@cmsimike cmsimike commented Apr 14, 2017 •
edited
Loading

Description:

Currently, rest_command does not allow you to config the request content type, which runs afoul of more strict api end points.

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2430

Example entry for configuration.yaml (if applicable):

# Example configuration.yaml entry
rest_command:
  example_request:
    url: 'http://example.com/'
    content_type: 'text/plain'

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@cmsimike
exposed content_type in rest_command, which allows for manually speci…
9925888 
…fying the content_type for more-strict api endpoints
@mention-bot
Copy link

@cmsimike, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @robbiet480 and @fabaff to be potential reviewers.

@homeassistant
Copy link
Contributor

Hi @cmsimike,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@cmsimike cmsimike mentioned this pull request Apr 14, 2017
Merged
houndci-bot
houndci-bot reviewed Apr 14, 2017
View reviewed changes
homeassistant/components/rest_command.py Outdated
@@ -13,7 +13,8 @@

from homeassistant.const import (
CONF_TIMEOUT, CONF_USERNAME, CONF_PASSWORD, CONF_URL, CONF_PAYLOAD,
CONF_METHOD)
CONF_METHOD, CONF_CONTENT_TYPE, CONTENT_TYPE_TEXT_PLAIN, HTTP_HEADER_CONTENT_TYPE,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (86 > 79 characters)

cmsimike added 2 commits April 14, 2017 10:19
@cmsimike
fixed up column length
7a93a1a 
Length was 86 chars, and it needed to be 79
@cmsimike
double import of HTTP_HEADER_CONTENT_TYPE
ee5aff5 
Removed the accidental double-import of HTTP_HEADER_CONTENT_TYPE
pvizeli
pvizeli requested changes Apr 14, 2017
View reviewed changes
Copy link
Member
@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It need also unittests...

homeassistant/components/rest_command.py Outdated
@@ -72,6 +74,9 @@ def async_register_rest_command(name, command_config):
template_payload = command_config[CONF_PAYLOAD]
template_payload.hass = hass

content_type = command_config[CONF_CONTENT_TYPE]
headers = {HTTP_HEADER_CONTENT_TYPE: content_type}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not set a default content type and force overwrite aiohttp internals. So if no type is get we should set headers None

homeassistant/const.py Outdated
@@ -76,6 +76,7 @@
CONF_COMMAND_STATE = 'command_state'
CONF_COMMAND_STOP = 'command_stop'
CONF_CONDITION = 'condition'
CONF_CONTENT_TYPE = 'content_type'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move that to component

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, you mean to move this into rest_command.py?

@cmsimike
Copy link
Contributor Author

I hope to address these comments tomorrow as I am about to step out for the night. Thanks!!

houndci-bot
houndci-bot reviewed Apr 16, 2017
View reviewed changes
tests/components/test_rest_command.py
self.hass.block_till_done()

assert len(aioclient_mock.mock_calls) == 1
assert aioclient_mock.mock_calls[0][2] == b'item'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no newline at end of file

@balloobbot balloobbot removed the core label Apr 16, 2017
@cmsimike
Copy link
Contributor Author

@pvizeli I believe your concerns have been addressed. I am not sure if there is a more official way for me to mark this as done.

cmsimike
cmsimike commented Apr 16, 2017
View reviewed changes
Copy link
Contributor Author
@cmsimike cmsimike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

pvizeli
pvizeli requested changes Apr 16, 2017
View reviewed changes
homeassistant/components/rest_command.py Outdated
headers = None
if CONF_CONTENT_TYPE in command_config:
content_type = command_config[CONF_CONTENT_TYPE]
headers = {HTTP_HEADER_CONTENT_TYPE: content_type}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/hdrs.py#L45 so we are sure that we right expose it to api

@cmsimike
Copy link
Contributor Author

Done 2: code boogaloo

pvizeli
pvizeli approved these changes Apr 18, 2017
View reviewed changes
Copy link
Member
@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚡

@pvizeli pvizeli merged commit 226066e into home-assistant:dev Apr 18, 2017
@balloob balloob mentioned this pull request Apr 21, 2017
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Jul 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@pvizeli pvizeli pvizeli approved these changes

Assignees
No one assigned
Labels
cla-signed component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cmsimike @mention-bot @homeassistant @houndci-bot @pvizeli @balloobbot
0