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

Already on GitHub? Sign in to your account

Add a cssselect method to all elements, not just HtmlElement #48

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

SimonSapin commented Apr 23, 2012

Submit #47 again, without all the clutter from #46.

This is a partial fix for #45. (Elements get a cssselect() method but are still not known to be HTML. You might need to pass translator='html'.)

@scoder already commented on two downsides:

  • It suggests bad practices for efficiency (the selector is compiled on every call)
  • It is too tightly integrated for something that needs an optional, external library

I think that both of these issues can be addressed with documentation, and the convenience can be worth it in some cases. (For example, pre-compiling will not help a selector that is used only once in a test suite.)

@SimonSapin SimonSapin Add a cssselect method to all elements, not just HtmlElement
translator defaults to 'xml' in _Element and 'html' in HtmlElement

(Add it back again, but on a separate pull request.)
fcd1dd0
Owner

scoder commented Sep 23, 2012

I'm rejecting this because I think that making cssselect an optional external package would leave a dead method in the API for users who do not have cssselect installed. It's pure convenience anyway, even in lxml.html.

@scoder scoder closed this Sep 23, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment