ENH: Added lxml-liberal html parsing flavor
State: closed

Closes #5130


Labels:

Comments:
@cpcloud Fixed

Again, I think you can handle the whole thing with a lambda and without the long docstring (just add a note about what strict means to `_LxmlFrameParser`).  What's the interface for the parsers? Are they all 3 attribute + potentially strict as a keyword, or can they have more?

It seems like a good idea for there to be a general way of passing setting to the underlying parser.

What other settings are there that you'd need? I'd like to keep this free of settings and have it "just work" for _most_ tables ... @cancan101 Your tables happen to be very difficult (which is great for testing), but for the majority of cases, tweaking the parser is too low-level for most users IMHO

What about changing the `parse_tables` tables call to take `match` and  `attrs` and leaving just `io` and any other parser settings on the init method? 

@cpcloud As of now, there are no other settings that I can think of. 

@jtratner @cpcloud  Heck, you could even move `io`,  `match` and `attrs` all to `parse_tables` and leave the `__init__` for JUST the parser settings (ie zero arguments for most classes).

I am not sure what you get by saving off:

```
        self.io = io
        self.match = match
        self.attrs = attrs
```

in the constructor. It means that you have to remake the parser for each new `io`. 

You could instead change this:

```
       p = parser(io, compiled_match, attrs)

        try:
            tables = p.parse_tables()
```

to:

```
       p = parser()

        try:
            tables = p.parse_tables(io, compiled_match, attrs)
```

not sure i totally follow you .... why don't you put up some code and we can take a look

The idea was to move around when data is passed to the parser. The parser would then be constructed with settings and then the parse specific data (for example `io`) would be passed on the call to `parse_tables`. This would make it more clear what is a setting for the parser and what is the data the parser is applied to.

It is not central to the point about `strict` just some musings on how to clean up the parser API.

oh i c...fine with me ... just can't break any existing tests!

@cpcloud Let me know what you think of the API change. Tests should pass (without any changes needed).

If this is good, i will squash and rebase.

bump?

@cpcloud Let me know if there is anything left to do here? Then I can squash and rebase.

bump ?

bump ?

Any way to get this in 0.13?

@cpcloud ?

@cancan101 Sorry for the long delay. Been crazy busy.

After mulling this over I'm going to have to say I'm :-1: on accepting this.

I think that a parser that gives unpredictable output is limited in its utility, mainly because of the nature of the errors that are seen. Allowing that unpredictability is only going to create headaches for maintainers.

My reasoning is a bit hand-wavy because it boils down to how much I trust these libs. I trust `html5lib` + `bs4` more than I trust `lxml`, simply from my experience using them. Keep in mind that a it is up to the implementer(s) of an HTML parser to decide what happens when a parse fails.

For example, `lxml` drops tags without warning (by default, no less) from [the FDIC bank list data set](http://www.fdic.gov/bank/individual/failed/banklist.html), where `htmllib` + `bs4` does what you would expect. `lxml` works fine on HTML such as that from `DataFrame.to_html()` (technically not valid HTML), and on valid HTML.

I actually don't think `lxml` is useful unless you're using it to parse XML, which in my experience is more likely to be correct (FWIW, I used to think `lxml` was the bee's knees until I had to spend some time figuring out why it wasn't giving me sane output from admittedly insane input). I would be in favor of removing `lxml` entirely, but for back compat we can't.

I _really_ _really_ hope this does **not** discourage you from contributing more. You could certainly take the parser cleanup you've done here and open a separate PR!

One of my concerns with bs4 is that on certainly pages, linked to on either
this pr or another issue, it enters an infinite loop.

As for unpredictability of results, I'm not sure that should be the
deciding factor. I can only imagine when and if a pdf parser is written in
that regard.

If this pr does not get accepted than at the very least I do want to make
sure that I can use this same code from "user land" because I do need it to
parse pages that the other parsers can't handle.
On Oct 29, 2013 8:47 PM, "Phillip Cloud" notifications@github.com wrote:

> @cancan101 https://github.com/cancan101 Sorry for the long delay. Been
> crazy busy.
> 
> After mulling this over I'm going to have to say I'm [image: :-1:] on
> accepting this.
> 
> I think that a parser that gives unpredictable output is limited in its
> utility, mainly because of the nature of the errors that are seen. Allowing
> that unpredictability is only going to create headaches for maintainers.
> 
> My reasoning is a bit hand-wavy because it boils down to how much I trust
> these libs. I trust html5lib + bs4 more than I trust lxml, simply from my
> experience using them. Keep in mind that a it is up to the implementer(s)
> of an HTML parser to decide what happens when a parse fails.
> 
> For example, lxml drops tags without warning (by default, no less) from the
> FDIC bank list data sethttp://www.fdic.gov/bank/individual/failed/banklist.html,
> where htmllib + bs4 does what you would expect. lxml works fine on HTML
> such as that from DataFrame.to_html() (technically not valid HTML), and
> on valid HTML.
> 
> I actually don't think lxml is useful unless you're using it to parse
> XML, which in my experience is more likely to be correct (FWIW, I used to
> think lxml was the bee's knees until I had to spend some time figuring
> out why it wasn't giving me sane output from admittedly insane input). I
> would be in favor of removing lxml entirely, but for back compat we can't.
> 
> I _really_ _really_ hope this does _not_ discourage you from contributing
> more. You could certainly take the parser cleanup you've done here and open
> a separate PR!
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/5131#issuecomment-27357008
> .

Additionally, if we add something, we then have to support it for the foreseeable future and allowing something that has undefined behavior is a big compatibility headache. We don't want to be responsible for lxml-liberal's output not changing if we make other changes to the module or how we approach the parsers.

Correct me if I'm wrong, but that unpredictability can be addressed in the docs. I know I have seen bs4 do unpredictable things (beyond the infinite loop, I can try to dig up an example). 

@cancan101 why don't u change this to allow flavor to be a class/callable and if it is then use that as the parser. then in user land u can subclass the current parse, pass it in, r and make it do what I want ?

I actually think I have an open issue to do just that. That solution is
fine as long as long the current lxml parser class is reasonably
extensible.
On Oct 29, 2013 9:03 PM, "jreback" notifications@github.com wrote:

> @cancan101 https://github.com/cancan101 why don't u change this to
> allow flavor to be a class/callable and if it is then use that as the
> parser. then in user land u can subclass the current parse, pass it in, r
> and make it do what I want ?
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/5131#issuecomment-27357701
> .

:+1: on allowing subclasses, then you can do whatever you want as long as you implement the interface

@cancan101 It should be fairly clear how to overload from the internal docs and the way the class is structured. I'm happy to answer questions to that effect.

@cpcloud The subclassing is easy (most of the work was done in this PR). The bulk of the remaining work is #4594.

as discussed above, not merging this.