Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jsomers opened this issue Oct 8, 2018 · 2 comments
Closed

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

jsomers opened this issue Oct 8, 2018 · 2 comments

Comments

@jsomers
Copy link

jsomers commented Oct 8, 2018

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 aantron closed this as completed in fd80378 Oct 9, 2018
@aantron
Copy link
Owner

aantron commented Oct 9, 2018

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
Copy link
Author

jsomers commented Oct 9, 2018

@aantron this is great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants