Media API #254
Media API #254
Conversation
slipeer
force-pushed
the
thumbnails
branch
3 times, most recently
from
d785c7c to
47bee9a
Compare
|
@Zil0 since you touch this in your E2E stuff, could you take a look at this to make sure there are no conflicts with how you're implementing things? |
matrix_client/api.py
Outdated
| ) | ||
|
|
||
| return response.json() | ||
| try: |
There was a problem hiding this comment.
It will be unclear why this is here without the context of the PR. More importantly, this doesn't feel very robust, what if the file to download is actually JSON?
I'd fix this by passing a new boolean to _send, not sure if this is the best way though.
matrix_client/api.py
Outdated
| raise ValueError("MXC URL did not begin with 'mxc://'") | ||
|
|
||
| def media_download(self, mxcurl): | ||
| """ Download raw media from provided mxc URL |
There was a problem hiding this comment.
You should get rid of the space after """, and add a . at the end of this line (applies to all of your docstrings).
matrix_client/api.py
Outdated
| method (str): thumb creation method. Must be | ||
| in ['scale', 'crop']. Default 'scale'. | ||
| """ | ||
| if method.lower() not in ['scale', 'crop']: |
There was a problem hiding this comment.
Not sure if tolerating improper case is useful, or consistent with the rest of the API methods.
|
All is fine, this will just simplify things a bit :) I've written down some thoughts I had when reading the code, but this is not a proper review. |
slipeer
force-pushed
the
thumbnails
branch
3 times, most recently
from
4dca37b to
ab48e0a
Compare
|
@Zil0 I fixed PR code for your comments. |
matrix_client/api.py
Outdated
| query_params={ | ||
| "width": width, | ||
| "height": height, | ||
| "method": method.lower() |
There was a problem hiding this comment.
lower is not needed anymore here.
matrix_client/api.py
Outdated
| Args: | ||
| mxcurl (str): mxc media URL | ||
| width (int): required thumbnail width | ||
| height (int): required thumbnail height |
There was a problem hiding this comment.
Might as well say "desired" instead of "required" here since the spec insists on this.
|
For completeness sake you may want to add the This is not needed for this to be merged though, just fix the conflicts and it will be good to go :) |
slipeer
force-pushed
the
thumbnails
branch
4 times, most recently
from
de39b44 to
16a2c32
Compare
|
Conflict removed. |
|
Thanks! |
3 participants
Methods
and tests for it.
Attemp to close issue #150
Signed-off-by: Pavel Kardash slipeer@gmail.com