io.html.read_html support XPath expressions for table selection
State: closed

First tentative PR as discussed in #5389


Labels:

Comments:
Could you hook up Travis? There's some instructions [here](https://github.com/pydata/pandas/blob/master/CONTRIBUTING.md#getting-travis-ci-going)

Just holler if you run into any issues.

I have a question concerning the first travis-ci build. In the docs [here](http://about.travis-ci.org/docs/user/getting-started/) it says that the first build has to be done with a push to the repo. Is there any other way? A dummy commit+push just for ci testing would be ugly and reforking, patching, PRing tedious... 

Can you just push this commit again? `git commit -C HEAD --amend` and then force push to this branch.

Build is running, thanks!

Just make sure you cover at least: invalid xpath, non-existent xpath, xpath
matching multiple, valid xpath matching None. Xpath that accidentally
references within a table (which should probably fail) and xpath refing
table directly. Looks like you've covered some of these cases.

I covered the needed test cases. I thought about "Xpath that accidentally references within a table" and generalized to "XPath does not return node list only containing tables". Please review.

Also, the travis build for python 2.6 fails. Importing `lxml.etree.XPathEvalError` fails with

```
ImportError: No module named lxml.etree
```

I never worked with 2.6.x Python lxml, so how to solve this?

Edit: direct [link](https://travis-ci.org/phaebz/pandas/jobs/13431882)

The 2.6 build doesn't have lxml, you need to guard your test cases to check if lxml is installed (look at other test cases /modules to see how this is done).

@phaebz so the contract for this is: "The XPath must return a set of table nodes and _only_ table nodes"?

@jtratner Yes, that was my idea.

Regarding lxml checking in tests, I will look into it.

it's `_skip_if_no('lxml')`

In the `setUpClass` method of `TestReadHtmlLxml` it says exactly that. This is why I originally thought that the `lxml` installed on the travis python 2.6 had a different module organisation, i.e. no lxml.etree.

Finally, all builds pass. What do you think?

don't u need to check if _HAVE_LXML is False and xpath is not None? (in the code I mean) and raise some kind of ValueError?

You probably want to skip the xpath tests if `import lxml` fails

@cancan101 See my post above, this should be taken care of in the `setUpClass` method of `TestReadHtmlLxml`.

@jreback I am not sure I understand you correctly. In _parse I raise a NotImplementedError if xpath is not None and flavor is one of bs4 or html5lib - so as to indicate that someone could implement it for these flavors if needed. Is this too indirect?

@phaebz He is saying that you never use or check `_HAS_LXML`

And that if `_HAS_LXML=False` and the user has set an xpath, there should be an error to indicate that parsing will not succeed. 

@phaebz what @cancan101 said!

I put this check into the _parse function, is there a better place?

Just to be clear, in my previous post I was referring to the check you proposed (xpath and `_HAS_LXML=False`) which was amended to the last commit.

@phaebz Now I see where `_HAS_LXML` is being checked, but now where is `_HAS_LXML` being set?

@cancan101 Isn't this done [here](https://github.com/phaebz/pandas/blob/read_html_xpath/pandas/io/html.py#L32-L37)?

Yes, my bad.

@phaebz can you squash down to a single commit?

@cpcloud ok?

@phaebz Needs release notes and an example or two. After that, good to go.

@phaebz ok...going to mark this for 0.13....

@cpcloud Did the easy/fast bits (not yet pushed). Will have a look and finish on the weekend.
@jreback yesss

One more thing: The fallback mechanism (lxml -> bs4) leads to a `NotImplementedError` in case the HTML is too broken/invalid for lxml parser. Now as long as xpath is not supported with bs4 flavor, the xpath feature is pretty much useless given all the broken HTML out there (I noticed this issue while preparing the examples, incidentally with the url I initially wanted to use the xpath with). That is unless we use the `recover=True` argument in lxml.html.HTMLParser instantiation. What do you think? One side effect of this more robust lxml HTML parsing is that tests which assert whether XMLSyntaxError has been raised (if called without fallback, i.e. `flavor=['lxml']`) will obviously fail.

@cpcloud Is there another important reason why you used the simple lxml parsing, i.e. `recover=False`, when you first wrote that part?

See #5131
On Nov 9, 2013 11:11 AM, "phaebz" notifications@github.com wrote:

> One more thing: The fallback mechanism (lxml -> bs4) leads to a
> NotImplementedError in case the HTML is too broken/invalid for lxml
> parser. Now as long as xpath is not supported with bs4 flavor, the xpath
> feature is pretty much useless given all the broken HTML out there (I
> noticed this issue while preparing the examples, incidentally with the url
> I initially wanted to use the xpath with). That is unless we use the
> recover=True argument in lxml.html.HTMLParser instantiation. What do you
> think? One side effect of this more robust lxml HTML parsing is that tests
> which assert whether XMLSyntaxError has been raised (if called without
> fallback, i.e. flavor=['lxml']) will obviously fail.
> @cpcloud https://github.com/cpcloud Is there another important reason
> why you used the simple lxml parsing, i.e. recover=False, when you first
> wrote that part?
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/5416#issuecomment-28130400
> .