Skip to content

Conversation

hjpotter92
Copy link
Contributor

I think instead of having a different method, it can be managed by the same upload call.

References #186.

This commit references maxtepkeev#186

Signed-off-by: hjpotter92 <hjpotter92+github@gmail.com>
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 8efbee2 on hjpotter92:issues/186 into ** on maxtepkeev:master**.

1 similar comment
@coveralls
Copy link

coveralls commented Sep 26, 2017

Coverage Status

Changes Unknown when pulling 8efbee2 on hjpotter92:issues/186 into ** on maxtepkeev:master**.

@maxtepkeev
Copy link
Owner

I don't see why we need additional method for this, just implement this logic inside existing upload method, also a test should be added to test this

 * The method first checks if the parameter is of type `file`,
  otherwise proceeds as before.
 * Define `url` and `headers` before any processing is done.

Signed-off-by: hjpotter92 <hjpotter92+github@gmail.com>
@coveralls
Copy link

coveralls commented Sep 27, 2017

Coverage Status

Changes Unknown when pulling a3df849 on hjpotter92:issues/186 into ** on maxtepkeev:master**.

Also allow for `StringIO` type variables

Signed-off-by: hjpotter92 <hjpotter92+github@gmail.com>
@coveralls
Copy link

coveralls commented Sep 27, 2017

Coverage Status

Changes Unknown when pulling 1a5b9f1 on hjpotter92:issues/186 into ** on maxtepkeev:master**.

@hjpotter92
Copy link
Contributor Author

@maxtepkeev For tests, I'd have to create a StringIO instance. I am not sure whether that is a good idea though!

@maxtepkeev
Copy link
Owner

@hjpotter92 Sorry for the delay with reply. Can you please elaborate why do you think it may be not a good idea ?

@hjpotter92
Copy link
Contributor Author

It'd require different test cases (or try-catch implementation) for different python versions; which I consider to be a hacky method.

@hjpotter92
Copy link
Contributor Author

@maxtepkeev Can you help me a little with writing the test case? I think I'll have to create an equivalent function to test_successful_file_upload? I could add the mock file data directly in the same function? What would the response be if I pass a file object?

@maxtepkeev
Copy link
Owner

@hjpotter92 Thanks again for the PR and sorry for such a delay with an answer, having hard times here... No worries about the test case, I'll write it myself.

@maxtepkeev maxtepkeev merged commit cbe5d48 into maxtepkeev:master Dec 8, 2017
@maxtepkeev maxtepkeev self-requested a review December 8, 2017 14:21
@hjpotter92
Copy link
Contributor Author

@maxtepkeev Thanks for merging the PR. 😄

BTW, I came across another bug because of this. The path keyword argument is being copied over to the final REST call. Since an object is now a valid value for the same, it raises an exception

.
.
.
File "/var/task/redminelib/engines/base.py", line 74, in request
kwargs = self.construct_request_kwargs(method, headers, params, data)
File "/var/task/redminelib/engines/base.py", line 57, in construct_request_kwargs
kwargs['data'] = json.dumps(data)
File "/usr/lib64/python2.7/json/__init__.py", line 244, in dumps
return _default_encoder.encode(obj)
File "/usr/lib64/python2.7/json/encoder.py", line 207, in encode
chunks = self.iterencode(o, _one_shot=True)
File "/usr/lib64/python2.7/json/encoder.py", line 270, in iterencode
return _iterencode(o, 0)
File "/usr/lib64/python2.7/json/encoder.py", line 184, in default
raise TypeError(repr(o) + " is not JSON serializable")
TypeError: <StringIO.StringIO instance at 0x7f141da69200> is not JSON serializable

For now, as a workaround; I added the following when the data is being jsonified:

            if 'attachments' in data:
                for attachment in data['attachments']:
                    attachment.pop('path')

@maxtepkeev
Copy link
Owner

@hjpotter92 Oh I see, ok, thx for letting me know.

avgas3 pushed a commit to avgas3/python-redmine that referenced this pull request Mar 31, 2021
Add globally available method to allow for uploading binary object
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants