Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Handle cases when inspect.stack() fails #582

Merged
merged 2 commits into from Feb 7, 2014

Conversation

@kmike
Copy link
Member

@kmike kmike commented Feb 4, 2014

Ricardo @panaggio reported an error: creating BaseSpider class fails if the first import is inside jinja2 template. Stacktrace:

[redacted]
  2. from slybot.spider import IblSpider
File "/usr/lib/pymodules/python2.7/slybot/spider.py" in <module>
  9.     from scrapy.spider import Spider
File "/usr/lib/pymodules/python2.7/scrapy/spider.py" in <module>
  68. BaseSpider = create_deprecated_class('BaseSpider', Spider)
File "/usr/lib/pymodules/python2.7/scrapy/utils/deprecate.py" in create_deprecated_class
  98.     frm = inspect.stack()[1]
File "/usr/lib/python2.7/inspect.py" in stack
  1054.     return getouterframes(sys._getframe(1), context)
File "/usr/lib/python2.7/inspect.py" in getouterframes
  1032.         framelist.append((frame,) + getframeinfo(frame, context))
File "/usr/lib/python2.7/inspect.py" in getframeinfo
  1007.             lines, lnum = findsource(frame)
File "/usr/lib/python2.7/inspect.py" in findsource
  580.             if pat.match(lines[lnum]): break

Exception Type: IndexError at /p/1844/spiders/
Exception Value: list index out of range

lines list contain lines of jinja2 template, not the python source code in this case, and it looks like inspect.stack() gets confused. In this PR such errors are catched and converted to warnings.

@kmike
Copy link
Member Author

@kmike kmike commented Feb 4, 2014

I think we shouldn't merge it without a test case.

@dangra
Copy link
Member

@dangra dangra commented Feb 5, 2014

can we add a valid testcase without installing jinja2?

@kmike
Copy link
Member Author

@kmike kmike commented Feb 5, 2014

It sems that this fails because jinja2 does some heavy magic with python code frames. Installing jinja2 looks easier than trying to find out what is this magic and how to replicate it.

@dangra
Copy link
Member

@dangra dangra commented Feb 5, 2014

but if jinja2 (or any other library) stops doing its bad magic the bug won't be properly tested. We want to test that when inspect.stack fails it won't panic for users.

@kmike
Copy link
Member Author

@kmike kmike commented Feb 5, 2014

We can just mock inspect.stack to raise an exception then.

I was thinking of adding jinja2 test because I haven't reproduced this issue myself yet. To reproduce it I was going to make a small example with jinja2, and why not put it to the testing suite then?

@panaggio
Copy link

@panaggio panaggio commented Feb 5, 2014

Adding jinja2 to scrapy's dependency (even if just test) seems too much to me.

I have tried to reproduce that myself too but couldn't either till now.

@kmike
Copy link
Member Author

@kmike kmike commented Feb 5, 2014

I personally don't care much about testing dependencies - they are installed automatically by tox or by travis, why should we worry about adding some extra ones?

@dangra
Copy link
Member

@dangra dangra commented Feb 5, 2014

well, is more work for travis too where jinja2 (and its deps) will be
installed per build taking longer to complete our already slow testsuite.

I am cool with testing by mocking inspect.stack

On Wed, Feb 5, 2014 at 3:07 PM, Mikhail Korobov notifications@github.comwrote:

I personally don't care much about testing dependencies - they are
installed automatically by tox or by travis, why should we worry about
adding some extra ones?


Reply to this email directly or view it on GitHubhttps://github.com//pull/582#issuecomment-34211512
.

@dangra dangra added the bug label Feb 5, 2014
dangra added a commit that referenced this pull request Feb 7, 2014
[WIP] Handle cases when inspect.stack() fails
@dangra dangra merged commit 45c9dab into scrapy:master Feb 7, 2014
@kmike kmike deleted the kmike:inspect-workaround branch Jun 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants