search

celery / vine

Python promises
Other
124 stars 43 forks source link

getfullargspec Python 2 compatibility fix #18

Closed jmartinm closed 6 years ago

jmartinm commented 6 years ago

Fixes https://github.com/celery/celery/issues/3993

jmartinm commented 6 years ago

Tests are failing for unrelated reasons to the PR. Let me know if there is some test that could be added.

thedrow commented 6 years ago

I don't understand how this change fixes the problem.

jmartinm commented 6 years ago

I don't understand how this change fixes the problem.

The problem I am trying to solve originates in this line https://github.com/celery/celery/blob/master/celery/contrib/sphinx.py#L52 when using Python 2.x since with the current behaviour, getfullargspec will return a structure that is compatible with Python 3.x (https://docs.python.org/3/library/inspect.html#inspect.getfullargspec) and not Python 2.x as it should.

With this PR, nothing will be changed in Python 3.x, but in Python 2.x (which makes the except run) getfullargspec would act like https://docs.python.org/2/library/inspect.html#inspect.getargspec which makes the original issue work.

thedrow commented 6 years ago

I cannot accept this change since getfullargspec is used elsewhere as well. See https://github.com/celery/celery/search?utf8=%E2%9C%93&q=getfullargspec&type= If the problem is with Sphinx, let's make the compatibility fallback there.

jmartinm commented 6 years ago

I cannot accept this change since getfullargspec is used elsewhere as well. See https://github.com/celery/celery/search?utf8=%E2%9C%93&q=getfullargspec&type= If the problem is with Sphinx, let's make the compatibility fallback there.

Yes, I was thinking where the appropriate place would be. I agree I would not want to cause any other side effect so will create a PR to do the change in https://github.com/celery/celery/blob/master/celery/contrib/sphinx.py

jmartinm commented 6 years ago

By the way, I just found this commit (https://github.com/celery/celery/commit/b0a9990ead4132f07268f7abadb4887c6f93f0f6#diff-ab5c8fa61a58d9064e3f1bd78c4f553f) that was done by @ask while five.py was still part of celery and that looks like the change I did in this PR.

Instead, this other commit (https://github.com/celery/vine/commit/cf28524c1b1bbea5bc3ab1d4f070feb842e12131#diff-ea0a91d012079a6d42db9fe589263712) in vine introduced the current code which IMHO is not correct.

jmartinm commented 6 years ago

Continued in https://github.com/celery/celery/pull/4399