Let extended translators override what XPathExpr class is used

[dangra]

GenericTranslator offers an excelent way to support custom selectors
trough method hooks and allowing to return a new XPathExpr from this
hooks.

The main problem is that returning extended XPathExpr instances fail
for combiners because XPathExpr.join() assume a fixed XPathExpr
instance attributes (element, path and condition) to copy from other to self

XPathExpr.join() can be extended in subclass but needs that left
xpath instance to be of the extended class too, and right now we can
only control right xpath type.

The problem can be mitigated by recasting all xpath returned from
GenericTranslator.xpath_element() that only works because it is the
only hook that cast XPathExpr instances.

The proposed change allow projects extending GenericTranslator to also
safely extend XPathExpr to correctly support combiners in extended
features.

[dangra]

To give some real context to this pull request you must know I am working on adding CSS selectors to Scrapy project based on cssselect

right now CSS can't select xpath text nodes or attributes and we want to keep this functionality within the selector query

I found GenericTranslator can be extended trough hook (great!) but found some quirks trying to implement it,
see scrapy/selector/csssel.py

and example translations at scrapy/tests/test_selector_csselect.py

thanks!

[SimonSapin]

This change looks easy enough, but I’m reluctant to encourage users to hook into the translators and even more so XPathExpr because backward-compatibility concerns effectively freeze the API.

However I am not confident at all in XPathExpr as an API to be frozen. It basically hasn’t changed yet since I inherited it from lxml.cssselect, but I think that it will need to change in order to properly implement CSS Selectors. See #4, #9, #12 and #18, among others.

I might still accept this change, but use this kind of hook at your own risks. In particular you might want to have cssselect==0.7.1 rather than just cssselect in your requirements, as 0.8 or 1.0 will probably break you program.

[dangra]

I just found pyquery uses cssselect, it extends cssselect.HTMLTranslator and XPathExpr, but pyquery devs took a monkeypatch approach https://github.com/gawel/pyquery/blob/master/pyquery/cssselectpatch.py#L230

Do you think the change is good for pyquery too @gawel?

[SimonSapin]

Well, all of the warnings above also apply to pyquery. They should at least consider requiring cssselect<0.8.

Overall I’m not sure how to manage this whole situation.

[dangra]

hi @SimonSapin, I am more worried about cssselect removing its extensibility than breaking its API.

I took a look to lselect and there isn't a clear way on how to extend it,.
do you plan to remove all form of hooks/extensibility after cssselect 0.8?

[gawel]

No problem. I'll just have to check if the class attribute exist and use the evil monkey patch iif it's needed

[dangra]

@gawel: the change is pretty simple and much better than monkeypatching, Simon is OK to merge it if we take the risk of changes to cssselect breaking our projects code. This is something we already do by extending undocumented/unstable GenericTranslator and XPathExpr classes.

@SimonSapin: sorry for insisting, my only concern on using cssselect is if you plan to completely remove its extensibility in future release, just breaking current apis doesn't matter to me.

thanks!

[SimonSapin]

@dangra I’m not planning on removing the extensibility, just keeping it undocumented and reserve the right to break it at every minor version until I’m confident that the API is good enough to be frozen.

(By beaking I mean changing it in backward-incompatible ways, not removing features or extensibility.)

As to lselect, it’s a two-weekends hack that is still highly experimental and probably broken. I’m not sure yet if it’ll be extensible or not, but in any case it’s completely separated from cssselect. cssselect won’t go away.

[dangra]

cool 👍

[SimonSapin]

See eac05a4