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

Distinguish between different types of parse exceptions #35

Closed
blicksky opened this issue Jul 2, 2014 · 4 comments
Closed

Distinguish between different types of parse exceptions #35

blicksky opened this issue Jul 2, 2014 · 4 comments

Comments

@blicksky
Copy link

blicksky commented Jul 2, 2014

I'm using your library to take URLs supplied by users, which may contain invalid syntax such as spaces, and convert them to valid URIs with URL.parse(it).toJavaURI().

One additional case that I'd like to cover is when the user leaves out the scheme. In this case, I would like to default it to http. Currently, I'm doing this by catching GalimatiasParseException, and checking to see if the exceptions's message is "Missing scheme".

This is working well for me, but checking the exception's message is very brittle. I'd like to suggest that there be a few subclasses of GalimatiasParseException, including something like MissingSchemeException, so that it can be captured in a safer way.

I'd be happy to submit a pull request if you think that this is a worthwhile enhancement.

@smola
Copy link
Owner

smola commented Jul 2, 2014

Having more fine-grained feedback about parsing errors is something I definitely want, but I haven't come up with a proper API for it yet. I'm more inclined to use a property of GalimatiasParseException with an enum with all possible errors, which can be conveniently handled in switch statements. If you want to submit a pull request, I'll be happy to review and merge it.

It would be interesting to support defaulting to http scheme too (an independent matter though), which is something I would also be happy to add if there's a sane API and behaviour for it.

Thank you for taking the time to contribute!

@blicksky
Copy link
Author

blicksky commented Jul 3, 2014

I took a first pass at adding an enum for parse issues to be included with GalimatiasParseException. I haven't fully tested it yet, but let me know if you have any thoughts or suggestions. I won't be able to get back to it until next week, but I'd appreciate any feedback.

https://github.com/blicksky/galimatias/compare/%2335-detailed-error-messages?expand=1

I had the thought that including the URL segment (scheme, host, path, etc.) where the parse error occurred would be another useful piece of information to have in the exception, but I don't have a use case for that at the moment. I also agree that supporting a default scheme (perhaps as part of URLParsingSettings?) would be a great feature.

@smola
Copy link
Owner

smola commented Jul 7, 2014

@blicksky Great! I would change "raise" to "throw" in method names, just to be more consistent with Java terminology. Could you add unit tests for the changes? You can do it in a new class (e.g. ParseIssueTest).

@smola
Copy link
Owner

smola commented Jul 8, 2014

Almost done in branch feature/issue-35 after merging #37.

@smola smola closed this as completed in 09ef443 Aug 18, 2014
sideshowbarker pushed a commit to validator/galimatias that referenced this issue Dec 13, 2015
sideshowbarker added a commit to validator/galimatias that referenced this issue Dec 13, 2015
sideshowbarker added a commit to validator/galimatias that referenced this issue Dec 13, 2015
sideshowbarker added a commit to validator/galimatias that referenced this issue Dec 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants