image attachment support #5

fracai · 2018-01-24T03:57:13Z

Pushover today added support for image attachments.
This PR adds support for specifying the path to an image when calling send_message.

send_message(…, image_path="/path/to/the/image")

codecov-io · 2018-01-24T04:01:35Z

Codecov Report

Merging #5 into develop will decrease coverage by 3.96%.
The diff coverage is 55.55%.

@@             Coverage Diff             @@
##           develop       #5      +/-   ##
===========================================
- Coverage      100%   96.03%   -3.97%     
===========================================
  Files            3        3              
  Lines          100      101       +1     
  Branches         7        8       +1     
===========================================
- Hits           100       97       -3     
- Misses           0        3       +3     
- Partials         0        1       +1

Impacted Files	Coverage Δ
src/pushover_complete/pushover_api.py	`95.34% <55.55%> (-4.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9c15f3...d15f584. Read the comment docs.

cthoyt · 2018-01-24T09:33:18Z

src/pushover_complete/pushover_api.py

 :param session: A :class:`requests.Session` object to be used to send HTTP requests.
 :type endpoint: str
 :type url_parameter: str
 :type payload: dict
+ :type files: dict


can you specify further what sort of dict this is? More specifically what are they key and value types?

Documentation from Pushover is here: https://pushover.net/api#attachments
And I used this page from the Requests documentation: http://docs.python-requests.org/en/latest/user/quickstart/#post-a-multipart-encoded-file

Basically, the dict is of the form:
{'attachment': (file_name, open(file_path, 'rb'))}
key must be 'attachment'
value is a tuple of the name of the file (doesn't seem to actually be used by Pushover) and the file object
You can also specify the Content-Type of the attachment, but it's not required and made the code simpler where I didn't need to determine what that would be. Though there's probably a very easy way to determine that for a given file.

so maybe better dict[str,tuple[str,BytesIO]] or whatever the appropriate type is for BytesIO since that definitely isn't it. PyCharm introspects on these and gives nice type hints :)

Thanks.
type(open(file_path, 'rb')) returns <type 'file'> so: dict[str,tuple[str,file]]?

It depends on how requests is implemented - the file class is a much more specific thing that also acts like an IO, so I'm not sure. Maybe look into the requests documentation? This is sort of a nit-picking thing so it's not that big of a deal

good point. Another acceptable form of the dictionary is to just pass in a string as the file content instead of using open().
Requests specifies it like this:

:param files: (optional) Dictionary of ``'name': file-like-objects`` (or ``{'name': file-tuple}``) for multipart encoding upload. ``file-tuple`` can be a 2-tuple ``('filename', fileobj)``, 3-tuple ``('filename', fileobj, 'content_type')`` or a 4-tuple ``('filename', fileobj, 'content_type', custom_headers)``, where ``'content-type'`` is a string defining the content type of the given file and ``custom_headers`` a dict-like object containing additional headers to add for the file.

I suppose I could just copy that.

https://github.com/requests/requests/blob/2872f17ab86583294907af84b9c36e63cd9a91ad/requests/api.py#L26

I would actually be happy to pass in some sort of iterable of file name(s) (or perhaps just a str/Path and only allow one file?) and leave the creation of the dict and the opening of the file to inside the _generic_post function. Forcing the user to pass in a dict with a specific key just seems like extra work for them that could be easily abstracted away by the library.

Only a single file can be attached to a given message, so I don't think it makes sense to use an iterable.
Also, the user would:

call send_message(..., image_path, ...) which then

call _send_message(..., image_path, ...); which performs the open, constructs the dict, and then

call _generic_post(..., files_dict)

Doesn't that keep the user from passing in a dict? It seemed like _send_message was the right place to put the dict construction.

I've cleaned up the param and type descriptions. It's still a little awkward, if anyone has suggestions for better wording I'm eagerly listening.

cthoyt · 2018-01-24T09:34:30Z

src/pushover_complete/pushover_api.py

+
+ file = {}
+ if image_path is not None and os.path.isfile(image_path):
+ file = {'attachment': (image_path, open(image_path, 'rb'))}


this doesn't handle file cleanup. Would using a context manager and calling read() be an appropriate alternative?

Good point. I whipped this together last night because I wanted to start pushing images. I didn't spend a ton of time thinking about the right way to do it.
I'll have to investigate though, Requests wants the file object and that would be cleaned up before the following call to post. It's probably enough to just put that call inside the with-open block.

Maybe this, then?

if image_path is not None and os.path.isfile(image_path): with open(image_path, 'rb') as f: file = {'attachment': (image_path, f)}

That's what I was thinking:

if image_path is not None and os.path.isfile(image_path): with open(image_path, 'rb') as f: file = {'attachment': (image_path, f)} return self._generic_post('messages.json', payload=payload, session=session, files=file)

Moving the open call inside _generic_post will handle this issue as well.

for now I still have the open call and dict construction inside _send_message. If you would prefer it move inside _generic_post I can do that. It seems to me to make more sense where it is, but I'm open.

cthoyt · 2018-01-24T09:35:18Z

src/pushover_complete/pushover_api.py

@@ -173,7 +183,7 @@ def send_message(self, user, message, device=None, title=None, url=None, url_tit
 :returns: Response body interpreted as JSON
 :rtype: dict
 """
- return self._send_message(user, message, device, title, url, url_title, priority, retry, expire, callback_url,
+ return self._send_message(user, message, device, title, url, url_title, image_path, priority, retry, expire, callback_url,


not specifically aimed at your PR, but I'd rather see kwargs used in this long call. it would make reading diffs easier too!

True, I briefly considered that, but didn't want to complicate the PR. I'd suggest a general issue tracking that across the whole project.

@scolby33 is going to go crazy and add type annotations so its 36+ only next time he does this, so be careful what you wish for :p

I'm still unsure on this. It seems to make discovery of the possible parameters more difficult...but I do admit that this signature is atrocious.

cthoyt · 2018-01-24T09:36:05Z

src/pushover_complete/pushover_api.py

@@ -162,6 +171,7 @@ def send_message(self, user, message, device=None, title=None, url=None, url_tit
 :type title: str
 :type url: str
 :type url_title: str
+ :type image_path: str


Should be str or pathlib.Path

good point; will change

cthoyt · 2018-01-24T09:37:16Z

src/pushover_complete/pushover_api.py

@@ -68,20 +71,19 @@ def _generic_post(self, endpoint, url_parameter=None, payload=None, session=None
 if payload is None:
 payload = {}
 payload['token'] = self.token
- headers = {'Content-Type': 'application/x-www-form-urlencoded'}


I'm curious: why was this here, and why is it no longer needed?

@scolby33 thoughts?

The documentation for (Pushing Messages)[https://pushover.net/api#messages] specifies that Content-Type in the example. Sending without an image seemed to work whether it was there or not. When sending the image attachment though the Content-Type needs to be multipart/form-data and that is added automatically. I think it must be the default when specify the data payload.
If the headers includes the urlencoded Content-Type though, it overrides the form-data specification and causes the post to fail.

I think I added this after doing some testing of the Pushover API with an external REST client that didn't add the application/x-www-form-urlencoded value. If Requests adds this by default as a guarantee, I'd love to see this line go. If not, we should have an if/else that puts the right header on depending on if there's an attachment or not. I should add sent header verification to the tests.

I'll double check how the call performs with and without the file parameter, but I think the header is indeed not required here.

I've tested with specifying a file and without and the application/x-www-form-urlencoded header is not needed.

If it works, I'm happy to have this removed! I've opened #7 to remind myself to add tests later just in case requests changes it's behavior someday.

cthoyt · 2018-01-24T09:37:52Z

Cool PR, @fracai. Excited to hear your feedback on my comments

fracai · 2018-01-24T14:21:08Z

Thanks, I've been asking for images to be supported for a while now and was excited to see that they've turned it on. I'm using your library to push ZoneMinder alerts and this makes them much more useful.
I should be able to get to these changes this evening.

scolby33

If these changes can get made, I'll merge this tonight/immediately.

I'll handle writing tests for the changes and make a minor release then, too.

scolby33 · 2018-01-28T18:01:08Z

src/pushover_complete/pushover_api.py

@@ -4,6 +4,7 @@
 from urlparse import urljoin

 import requests
+import os


I tend to keep imports in order of stdlib, then packages, then internal imports. Could you change to

import os try: ... except: ... import requests

Disagree. It is standard to have the try/except stuff at the end of imports

defering to the owner for now

scolby33 · 2018-01-28T18:04:20Z

src/pushover_complete/pushover_api.py

@@ -68,20 +71,19 @@ def _generic_post(self, endpoint, url_parameter=None, payload=None, session=None
 if payload is None:
 payload = {}
 payload['token'] = self.token
- headers = {'Content-Type': 'application/x-www-form-urlencoded'}


If it works, I'm happy to have this removed! I've opened #7 to remind myself to add tests later just in case requests changes it's behavior someday.

scolby33 · 2018-01-28T18:11:24Z

src/pushover_complete/pushover_api.py

+
+ if image_path is not None and os.path.isfile(image_path):
+ with open(image_path, 'rb') as f:
+ file = {'attachment': (image_path, f)}


I see in the requests docs that you can use a "file tuple" but it's not explained why the filename could be important. Do we need (image_path, f) or do we just want 'attachment': f?

The other thing to check is if image_path is a relative or absolute path, does requests want/deal with the full /Users/foo/whatever.png or does it really want just whatever.png? If the latter we should do something with os.path to extract just the part we want.

I assume the file name is used to specify a different name for downloading, but it doesn't look like Pushover is using this for anything; it is required in the case of the tuple. I suppose the tuple could be avoided and just pass in the file object.

Alternatively, os.path.split(image_path)[1] would be a way to just pass in the file name.

I like the idea of dropping the tuple to be honest.

scolby33 · 2018-01-28T18:12:22Z

src/pushover_complete/pushover_api.py

+ if image_path is not None and os.path.isfile(image_path):
+ with open(image_path, 'rb') as f:
+ file = {'attachment': (image_path, f)}
+ return self._generic_post('messages.json', payload=payload, session=session, files=file)


I really hate the need for an if here but I can't see a more elegant way to do it.

I could drop the conditional and just rely on exception handling, but that would still be:

try: with open( image_path, 'rb') as f : file = {'attachment': f} return self._generic_post('messages.json', payload=payload, session=session, files=file) except IOError: pass return self._generic_post('messages.json', payload=payload, session=session)

scolby33 · 2018-01-28T18:18:19Z

src/pushover_complete/pushover_api.py

@@ -173,7 +183,7 @@ def send_message(self, user, message, device=None, title=None, url=None, url_tit
 :returns: Response body interpreted as JSON
 :rtype: dict
 """
- return self._send_message(user, message, device, title, url, url_title, priority, retry, expire, callback_url,
+ return self._send_message(user, message, device, title, url, url_title, image_path, priority, retry, expire, callback_url,


I'm gonna request a semi-major change here: can the argument be changed to image and we allow either str or pathlib.Path or a BytesIO object? That would allow an image to be created in memory by the application and sent without ever writing it to disk. It would also allow the caller to manage the lifetime of the file handle.

This is just a change to the signature and docs of send_message but would require a bit more logic in _send_message to decide whether to open the file or just pass it along.

Shouldn't change _generic_post at all, I think.

what's the best way to do this and keep the code clean? Have two versions of the function that handle the file argument differently then the handler picks which strategy based on the type of the input?

Ah, yes. I can do that, but I probably won't get to it until this evening. I've pushed the other changes for now and I'll get to this as soon as I can.

I found the time to put it together right now. That said, I haven't tested it beyond passing in a string still works.

scolby33 · 2018-01-28T18:24:58Z

Also @fracai add yourself to AUTHORS.rst too!

cthoyt · 2018-01-28T21:54:05Z

src/pushover_complete/pushover_api.py

- file = {'attachment': f}
- return self._generic_post('messages.json', payload=payload, session=session, files=file)
+ if image is not None:
+ if isinstance(image, file) or isinstance(image, io.BytesIO):


breaks python2 compatibility. Use six to fix this import

cthoyt · 2018-01-28T21:54:34Z

src/pushover_complete/pushover_api.py

+ if isinstance(image, file) or isinstance(image, io.BytesIO):
+ attachment = {'attachment': image}
+ return self._generic_post('messages.json', payload=payload, session=session, files=attachment)
+ if isinstance(image, str) and os.path.isfile(image):


use six.string_types for python2/3 compatibility

What part breaks? I tested this with python2. I'm looking in to six as well.

Ah, str is only python 3? Strange that it worked for me. I'll add in six usage.

scolby33 · 2018-02-22T08:31:20Z

Superseded by #9

$fracai$

$@fracai$

added image attachment support

69786b8

cthoyt reviewed Jan 24, 2018

View reviewed changes

fracai added 3 commits January 24, 2018 21:06

$@fracai$

update to file dict description

b8718ab

$@fracai$

context manage opening image attachment

fd44aa9

$@fracai$

updating type descriptions for image_path

4fb6943

scolby33 requested changes Jan 28, 2018

View reviewed changes

fracai added 4 commits January 28, 2018 15:52

$@fracai$

re-ordering imports

47ca9b6

$@fracai$

no need to pass in the "filename" to requests

1be16c5

$@fracai$

added my contribution credit

19fd960

$@fracai$

support for passing in file-like object as the attachment

24806db

cthoyt reviewed Jan 28, 2018

View reviewed changes

$@fracai$

use six for 2/3 compatibility

d15f584

scolby33 mentioned this pull request Feb 22, 2018

Feature/image attachment support #9

Merged

scolby33 closed this Feb 22, 2018

image attachment support #5

image attachment support #5

Conversation

fracai commented Jan 24, 2018

codecov-io commented Jan 24, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthoyt Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthoyt commented Jan 24, 2018

fracai commented Jan 24, 2018

scolby33 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scolby33 Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scolby33 commented Jan 28, 2018

cthoyt Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scolby33 commented Feb 22, 2018

$@fracai$ fracai commented Jan 24, 2018

codecov-io commented Jan 24, 2018 •

edited

Loading

cthoyt Jan 24, 2018 •

edited

Loading

scolby33 Jan 28, 2018 •

edited

Loading

cthoyt Jan 28, 2018 •

edited

Loading