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

Idea: warn users when trying to use TextResponse functionality with plain Response #2264

Closed
eliasdorneles opened this issue Sep 19, 2016 · 3 comments
Labels

Comments

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented Sep 19, 2016

Currently, if we try to use TextResponse functionality like response.text or css()/xpath() methods with a plain Response (e.g. in case of binary content), we get an AttributeError:

>>> response.css
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-1-7d6e256164d4> in <module>()
----> 1 response.css

AttributeError: 'Response' object has no attribute 'css'
>>> response.xpath
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-4f61f6e9fc6e> in <module>()
----> 1 response.xpath

AttributeError: 'Response' object has no attribute 'xpath'
>>> response.text
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-be6a4a00df5e> in <module>()
----> 1 response.text

AttributeError: 'Response' object has no attribute 'text'

Would it make sense to add a few methods/properties to explain what's going on for new users?

I was thinking instead of AttributeError, a better behavior could be a ValueError with a message giving a bit more context.

So, in plain Response, we could have:

def css(self, *args, **kw):
    raise ValueError('Response content is not text')

def xpath(self, *args, **kw):
    raise ValueError('Response content is not text')

@property
def text(self, *args, **kw):
    raise ValueError('Response content is not text')

This would be nice, because we'd had to explain fewer things when teaching people about responses and also about using .css and .xpath methods.

What do you think?

@kmike
Copy link
Member

@kmike kmike commented Sep 19, 2016

This change could be backwards incompatible; code may rely on presence of these attributes. I'm using if hasattr(response, 'text') sometimes to check for response type without having to import scrapy.http.TextResponse.

@eliasdorneles
Copy link
Member Author

@eliasdorneles eliasdorneles commented Sep 19, 2016

Hmm, gotcha, you're relying on the AttributeError.
What if we kept the AttributeError exception for the text property, then?

@kmike
Copy link
Member

@kmike kmike commented Sep 19, 2016

Sounds good, that'd be just providing more explanatory error messages for AttributeErrors.

@redapple redapple closed this in 9c9690c Nov 10, 2016
redapple added a commit that referenced this issue Nov 10, 2016
[MRG+1] Add better messages for when response content isn't text (closes #2264)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.