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

#35 detailed error messages #37

Closed
wants to merge 1 commit into from

Conversation

blicksky
Copy link

@blicksky blicksky commented Jul 7, 2014

No description provided.

@blicksky
Copy link
Author

blicksky commented Jul 7, 2014

I added test coverage for the changes to GalimatiasParseException as well as for when the different types of ParseIssue are used with URLParser. I didn't cover every usage, mostly because I wasn't sure how to construct URLs that would cause each issue in every case, so let me know if you'd like to see this filled out even further. Also, let me know if you see a better way to test what I was trying to do in ParseIssueTest.

I had specifically avoided using method names like throwError and throwFatalError because an exception is not always thrown, depending on the ErrorHandler. I switched to use "handle", as in handleError and handleFatalError to avoid any confusion between overloaded terms like "raise" or "throw".

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.27%) when pulling 7f8da49 on blicksky:#35-detailed-error-messages into 649b1fd on smola:master.

this.parser.settings(URLParsingSettings.create().withErrorHandler(new ErrorHandler() {
@Override
public void error(GalimatiasParseException error) throws GalimatiasParseException {
errorException = error;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the error should be thrown so that parsing stops.

@smola
Copy link
Owner

smola commented Jul 7, 2014

Implementation and tests are good. I'll add some further cases later. Could you address my comments and squash all changes to a single commit?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.21%) when pulling 4140309 on blicksky:#35-detailed-error-messages into 649b1fd on smola:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.21%) when pulling e0fe1d2 on blicksky:#35-detailed-error-messages into 649b1fd on smola:master.

@smola
Copy link
Owner

smola commented Jul 8, 2014

Thank you @blicksky! I have merged this PR to a branch: https://github.com/smola/galimatias/tree/feature/issue-35

I will merge to master once I'm sure all cases are covered.

This pull request was closed.
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

Successfully merging this pull request may close these issues.

3 participants