ckan / ckanext-qa

CKAN QA Extension
MIT License
26 stars 52 forks source link

Exception thrown when url 404 #21

Closed KrzysztofMadejski closed 8 years ago

KrzysztofMadejski commented 8 years ago
[2015-10-26 11:48:29,819: ERROR/PoolWorker-2] qa.update[3e3cc069-23c7-4470-875e-4633c7f552db]: Exception occurred during QA update: AttributeError: 'Response' object has no attribute 'error'
[2015-10-26 11:48:29,820: INFO/PoolWorker-2] Starting new HTTPS connection (1): danepubliczne.gov.pl
[2015-10-26 11:48:29,861: ERROR/MainProcess] Task qa.update[3e3cc069-23c7-4470-875e-4633c7f552db] raised exception: AttributeError("'Response' object has no attribute 'error'",)
Traceback (most recent call last):
  File "/home/ckan/.virtualenvs/ckan/local/lib/python2.7/site-packages/celery/execute/trace.py", line 47, in trace
    return cls(states.SUCCESS, retval=fun(*args, **kwargs))
  File "/home/ckan/.virtualenvs/ckan/local/lib/python2.7/site-packages/celery/app/task/__init__.py", line 247, in __call__
    return self.run(*args, **kwargs)
  File "/home/ckan/.virtualenvs/ckan/local/lib/python2.7/site-packages/celery/app/__init__.py", line 175, in run
    return fun(*args, **kwargs)
  File "/home/ckan/.virtualenvs/ckan/src/ckanext-qa/ckanext/qa/tasks.py", line 162, in update
    _update_resource(context, data, log)
  File "/home/ckan/.virtualenvs/ckan/src/ckanext-qa/ckanext/qa/tasks.py", line 104, in _update_resource
    % (res.status_code, content, context, resource, res, res.error, post_data, api_url))
AttributeError: 'Response' object has no attribute 'error'
Traceback (most recent call last):
  File "/home/ckan/.virtualenvs/ckan/local/lib/python2.7/site-packages/celery/execute/trace.py", line 47, in trace
    return cls(states.SUCCESS, retval=fun(*args, **kwargs))
  File "/home/ckan/.virtualenvs/ckan/local/lib/python2.7/site-packages/celery/app/task/__init__.py", line 247, in __call__
    return self.run(*args, **kwargs)
  File "/home/ckan/.virtualenvs/ckan/local/lib/python2.7/site-packages/celery/app/__init__.py", line 175, in run
    return fun(*args, **kwargs)
  File "/home/ckan/.virtualenvs/ckan/src/ckanext-qa/ckanext/qa/tasks.py", line 162, in update
    _update_resource(context, data, log)
  File "/home/ckan/.virtualenvs/ckan/src/ckanext-qa/ckanext/qa/tasks.py", line 104, in _update_resource
    % (res.status_code, content, context, resource, res, res.error, post_data, api_url))
AttributeError: 'Response' object has no attribute 'error'
davidread commented 8 years ago

With newer requests versions, we need to change res.error to res.reason. Sorry I thought I'd sorted this. Could you do a PR?

KrzysztofMadejski commented 8 years ago

Just to be sure, I have requests==2.3.0, is this the right version?

davidread commented 8 years ago

Yep, 2.3 is fine. 2.7 also. The requirements are now requests>=1.1.0 so this should have been fixed previously, so thanks for the PR.

KrzysztofMadejski commented 8 years ago

Shouldn't we do requests~=2.3 ? Open bracket can lead to accidentally installing backwards-incompatible version.. (like 3.0)

davidread commented 8 years ago

Interesting, what does the tilde do here?

I see no reason to add an upper bound for theoretical versions. It's not as if we're distributing releases - people tend to get the latest code from github at the same time as they get the requirements, so if there is an incompatible requests version then we can deal with it then.

And requests 2.0 was backwards compatible for most use cases.

On 6 November 2015 at 10:44, Krzysiek Madejski notifications@github.com wrote:

Shouldn't we do requests~=2.3 ? Open bracket can lead to accidentally installing backwards-incompatible version.. (like 3.0)

— Reply to this email directly or view it on GitHub https://github.com/ckan/ckanext-qa/issues/21#issuecomment-154376673.

KrzysztofMadejski commented 8 years ago

Hm, I guess tilde is for different language than Python. I've meant http://stackoverflow.com/questions/8795617/how-to-pip-install-a-package-with-min-and-max-version-range

Specyfing max version is the safest solution. If you want to be up-to-date with all the dependencies through bug reporting, that's ok. I feel one should update dependency only when there's need to.

As for releases - shouldn't ckanext-qa go along with versioning of CKAN? That was my original problem with request - I had newer CKAN with newer requests and they had to match with this extension. But if somebody is on older CKAN version who won't be able to use latest code from this repo, and won't know which commit to use.

Correct me if I'm wrong.

davidread commented 8 years ago

Allow me to summarize. The specific use case we're discussing is when someone is newly installing an old version of ckanext-qa. You are concerned that since then an incompatible version of requests has been released which causes them problems. I'm concerned that they may wish to use a newer version of requests due to ckan or another extension wanting it, and an upper limit on QA might suggest it is not allowed or possible. So whether we add an upper limit is a balance of probabilities of which is more helpful for more people over time. You may be right, but it's difficult to argue either way definitely.

I prefer to encourage people to use the latest ckanext-qa and we all chip in to keep it up to date. Generally there's not been any interest in the original parties who designed and wrote ckanext-qa - OKF and data.gov.uk, hence my keenness to merge the continued effort from data.gov.uk back in in PR https://github.com/ckan/ckanext-qa/pull/24 for which I'd appreciate feedback.