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

selector.__repr__ test #779

Merged
merged 4 commits into from Jul 3, 2014
Merged

selector.__repr__ test #779

merged 4 commits into from Jul 3, 2014

Conversation

@Digenis
Copy link
Member

@Digenis Digenis commented Jul 2, 2014

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


self.assertEqual(
map(repr, sel.xpath('//input/@name')),
[u"<Selector xpath='//input/@name' data=u'{}'>".format(40 * 'b')]

This comment has been minimized.

@kmike

kmike Jul 2, 2014
Member

__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'.

This comment has been minimized.

@Digenis

Digenis Jul 3, 2014
Author Member

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.

@Digenis
Copy link
Member Author

@Digenis Digenis commented Jul 3, 2014

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

This comment has been minimized.

Copy link
Owner Author

@Digenis Digenis commented on aacf1d0 Jul 3, 2014

@kmike
Copy link
Member

@kmike kmike commented Jul 3, 2014

@Digenis it casts repr on _expr via %r format specifier.

@kmike
Copy link
Member

@kmike kmike commented Jul 3, 2014

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.

@Digenis
Copy link
Member Author

@Digenis Digenis commented Jul 3, 2014

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 will probably branch the other PR from here and squash/rebase later to skip commits with rejected parts.

@kmike
Copy link
Member

@kmike kmike commented Jul 3, 2014

I'm intrigued :P

kmike added a commit that referenced this pull request Jul 3, 2014
selector.__repr__ test
@kmike kmike merged commit a653bf3 into scrapy:master Jul 3, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@dangra
Copy link
Member

@dangra dangra commented Jul 3, 2014

+1 for increasing test coverage. 🚑

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