search

okfn / ckanclient-deprecated

DEPRECATED - please see https://github.com/ckan/ckanapi. [Python client library for CKAN]
http://pypi.python.org/pypi/ckanclient
25 stars 17 forks source link

Support non-ascii file upload with ckanclient #29

Closed Hoedic closed 10 years ago

Hoedic commented 10 years ago

Currently, ckanclient throws errors when trying to upload non-ascii file (mainly with latin1 encoding). This code proposes some changes to support any time of encoding as it does not "opening" the files to be uploaded. Also adds some exception management at the file upload level.

rufuspollock commented 10 years ago

@davidread would you be up for a review?

davidread commented 10 years ago

Looks like much simpler code using the pycurl library and more error handling, which is v. good. However I've not tried it and this part of ckanclient which although I've looked at previously, I've failed to understand - the ins and outs of multipart form encoding seem pretty obscure.

@slmnhq wrote the original code 57e86c33b , so perhaps he can comment on this change?

slmnhq commented 10 years ago

+1 for pycurl or requests.

I was trying to avoid introducing a new dependency beyond httplib.

On Fri, Nov 22, 2013 at 5:20 AM, David Read notifications@github.comwrote:

Looks like much simpler code using the pycurl library and more error handling, which is v. good. However I've not tried it and this part of ckanclient which although I've looked at previously, I've failed to understand - the ins and outs of multipart form encoding seem pretty obscure.

@slmnhq https://github.com/slmnhq wrote the original code 57e86c3https://github.com/okfn/ckanclient/commit/57e86c33b, so perhaps he can comment on this change?

— Reply to this email directly or view it on GitHubhttps://github.com/okfn/ckanclient/pull/29#issuecomment-29062572 .

Hoedic commented 10 years ago

I initially tried to improve the existing code without any news dependency but it appeared to be difficult to do (Note: I am far from an experienced developer). As a result, pycurl seemed to be a reasonable requirement given the task at end.

rufuspollock commented 10 years ago

No problem introducing a dep in my opinion but i'm wondering if http://www.python-requests.org/en/latest/ might not be better than pycurl as it would be pure python ...

rsignell-usgs commented 10 years ago

+1 for requests

rufuspollock commented 10 years ago

@Hoedic i know itis a bit of an ask but would you consider a switch for your patch to use requests - you'll probably like it even more :-)

Hoedic commented 10 years ago

Yes, I'll look at that. Let me few days because I have a rush for the moment

rufuspollock commented 10 years ago

@Hoedic so what i'm going to suggest is we merge your PR now (its a definite improvement) and then we try to refactor to use requests - i've opened an issue for this as #31