forked from lxml/lxml
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add a cssselect method to all elements, not just HtmlElement
translator defaults to 'xml' in _Element and 'html' in HtmlElement
- Loading branch information
1 parent
8c8418f
commit 7820b8d
Showing
2 changed files
with
16 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7820b8d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, my bad. I should have checked. I had thought this method was already there and needed fixing, but it was only in lxml.html, not in lxml.etree. I didn't mean to add it to lxml.etree.
7820b8d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that it should not be added there?
7820b8d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I'm saying, yes. In any case, it's an addition to the API that is independent of the intention of this pull request.
Is there a real advantage of having it on all Elements, instead of just HtmlElements? I mean, it's convenient, ok, but it's not efficient and also won't work on all systems because it requires an external module to be installed. Making it a method feels too tightly integrated for an external module.
I'm really not sure, but I'm leaning towards considering the method on HtmlElements a legacy issue rather than a sparkling good idea. And if that's the case, we shouldn't add to it.
7820b8d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I’ll make two independent pull requests so they can be judged separately.
7820b8d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks