aantron / lambdasoup

Functional HTML scraping and rewriting with CSS in OCaml
https://aantron.github.io/lambdasoup
MIT License
380 stars 31 forks source link

"cannot select" exception text is a bit misleading #22

Closed jsomers closed 5 years ago

jsomers commented 5 years ago

Following the examples in the README, I had soup $ ".classname" in my code and was surprised to see the exception "Exception: Failure "Soup.($): cannot select '.classname'"."

The message made me think I had done something wrong -- like I had used the wrong parser for my document, or like I was passing the wrong kind of selector to the function. ("Does LambdaSoup not take class names? Or maybe only when you parse your document as XML?")

If instead the message had read, e.g., "Exception: Failure "Soup.($): '.classname' not found"." I would have had a better idea of what was going on. The message could even suggest using $? if you don't want to raise when the element can't be found.

In hindsight, I would have avoided the confusion if I read the nicely-commented mli where $ appears, but alas, my way in to the library -- as I suspect might be the case for many people -- was the set of usage examples in the README. The name $ in itself gives no indication that an exception might be raised, so giving a more user-friendly exception (and perhaps a variant literally called Not_found, instead of Failure, in addition to a better message) seems especially important in this case.

aantron commented 5 years ago

Thanks for reporting! The message is now:

Soup.($): '.classname' not found.
Try Soup.($?) if you'd prefer returning None instead of exceptions.

I didn't change the exception constructor, because there is a built-in Not_found that takes no arguments, and I preferred not to define a new exception for this, for now.

my way in to the library -- as I suspect might be the case for many people -- was the set of usage examples in the README

Agreed :)

jsomers commented 5 years ago

@aantron this is great, thanks!