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

Remove XPath string() conversion of element in CSS "contains" function translation #30

Merged
merged 1 commit into from
Sep 15, 2013

Conversation

redapple
Copy link
Contributor

According to http://www.w3.org/TR/xpath/#section-Function-Calls (and in practice using lxml), each argument is "converted (...) to the type required by the function,".

So, the string(.) conversion on the context node is redundant.
Very minor, and probably only cosmetic.

@SimonSapin
Copy link
Contributor

Thanks for contributing. I’m ok with this change, but please:

  • Change the Travis file in a separate commit
  • Avoid unrelated whitespace change, or if you think they’re important do them in a separate commit.

@redapple
Copy link
Contributor Author

Sorry for the multiple commits, I got mixed up in my reverts.
Any way to clean that up?

@redapple
Copy link
Contributor Author

Rebased to make things cleaner.

@SimonSapin
Copy link
Contributor

Merged, thanks.

@SimonSapin SimonSapin merged commit 9fff95b into scrapy:master Sep 15, 2013
@redapple
Copy link
Contributor Author

De rien Simon !
cssselect is pretty amazing, so I'm glad I can help out (even those small things)

@redapple redapple deleted the contains branch September 15, 2013 21:02
@SimonSapin
Copy link
Contributor

Well, I’ve grown to hate XPath but that’s what we have :) (So far…)

@redapple
Copy link
Contributor Author

Well, I'd be happy to discuss #29 to see how to properly extend cssselect translators for that extra bit "missing" in CSS ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants