selector.__repr__ test #779
Conversation
|
|
||
| self.assertEqual( | ||
| map(repr, sel.xpath('//input/@name')), | ||
| [u"<Selector xpath='//input/@name' data=u'{}'>".format(40 * 'b')] |
There was a problem hiding this comment.
__repr__ must return a bytestring in Python 2.x, not unicode, and for maximum compatibility it should be ascii-only.
So 'u' should be removed, and I think it is better to check what happened with '\xa9'.
There was a problem hiding this comment.
I think you mean: check what would happen with \xa9.
As a part of the selected data it shouldn't cause a problem because str() calls repr() on them.
I will add another assertion for it.
|
Don't merge yet. I noticed a potential bug while replying to kmike's review and I may add a failing test for it. |
|
@Digenis it casts repr on |
|
I'm +0 to merge this - yay tests! The downside is that unicode repr test will start to fail in Python 3 (because repr changed - it is no longer ascii-only), but this can be fixed by having two versions for this test in a future PY3-compatible Scrapy. |
|
Well, I explained in the PR message why I open this, I plan to open a PR for another issue on which I will tamper with those methods and I don't want to mix this into one. |
|
I'm intrigued :P |
|
+1 for increasing test coverage. 🚑 |
Some may consider it redundant, I won't argue.
But discussions and pull requests on selector's methods come and go these days (referring to extract_one, join etc) so I thought some extra coverage may prove useful later.
Which means I have a patch in mind and I need some test coverage before working on it and opening a PR