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

perform exception handling outside Parser.parse method #107

Merged
merged 5 commits into from
Aug 12, 2022

Conversation

brbog
Copy link

@brbog brbog commented Aug 8, 2022

-> allows more consistent use of config.isHaltOnError() (by default everything is logged, but overriding that behavior is easier now)
considering all checked and unchecked exceptions as parsing exceptions (was the de facto behavior)

 -> allows more consistent use of config.isHaltOnError() (by default everything is logged, but overriding that behavior is easier now)
considering all checked and unchecked exceptions as parsing exceptions (was the de facto behavior)
@brbog
Copy link
Author

brbog commented Aug 8, 2022

Removed the (new) method:
protected void onContentFetchError(Page page) {
in favor of the previously deprecated:
protected void onContentFetchError(WebURL webUrl) {
because a page object is not always available and only filled in part when entering this method.

Went all the way for the newer method:
protected void onParseError(WebURL webUrl, Exception e) {
(removed the deprecated one without Exception argument)
because it's very interesting to be able to log the stacktrace of a parsing exception.

@brbog
Copy link
Author

brbog commented Aug 8, 2022

I'll fix the build errors later today, but you can have a look already and give feedback if you want.

It started out after seeing possible non consequent exception handling inside the Parser where the config.isHaltOnError() is used for binary content, but nowhere else. There were multiple try catches everywhere which made things less readable. I also broke up some logic inside a couple of methods for readability.

@brbog
Copy link
Author

brbog commented Aug 8, 2022

Should be ok now, the most breaking changes are the "onError"-methods mentioned and the parse method that throws Exception instead of ParseException.

The reason I went for updating the exception handling is that I found a bug with the css parser when parsing some css code with the latest constructs, and I would like that the css file is still processed even if parsing it for outgoing urls fails. At the moment we're not there yet as I kept the behavior as it used to be (even if it looks a little different). Changing how we deal with parse errors inside css files together with fixing this inside ph-css should move things forward.

Copy link
Collaborator

@rzo1 rzo1 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. See my comments :)

@rzo1
Copy link
Collaborator

rzo1 commented Aug 12, 2022

Thanks for the explanations, @brbog !

Bram Bogaert added 2 commits August 12, 2022 11:34
@brbog
Copy link
Author

brbog commented Aug 12, 2022

Variable processed is final now.
Added protected void onContentFetchError(WebURL webUrl, Exception e) and set the previous 2 methods deprecated (having the Exception could be important as the method is called after a multi-catch).

@rzo1 rzo1 merged commit 9fa6b3c into HHN:master Aug 12, 2022
@brbog brbog deleted the exception-handling-and-halt-on-error branch August 12, 2022 19:32
@rzo1 rzo1 added this to the v4.10.1 milestone Aug 15, 2022
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.

2 participants