-
-
Notifications
You must be signed in to change notification settings - Fork 17
Add update block error handling #342
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
Conversation
…er; fix mis-ported lines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #342 +/- ##
==========================================
- Coverage 72.32% 72.28% -0.04%
==========================================
Files 416 417 +1
Lines 35438 35467 +29
Branches 4897 4900 +3
==========================================
+ Hits 25629 25638 +9
- Misses 8711 8731 +20
Partials 1098 1098 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ddaspit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ddaspit reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion
src/SIL.Machine/Corpora/PlaceMarkersUsfmUpdateBlockHandler.cs line 42 at r1 (raw file):
/// <param name="errorHandler">Error handler should return true if parsing should continue; error handler should return false if exception should be thrown. Default behavior is to rethrow. </param> /// </summary> public PlaceMarkersUsfmUpdateBlockHandler(Func<UsfmUpdateBlockHandlerException, bool> errorHandler = null)
It would be more generic if we pass in the error handler to the UpdateUsfmParserHandler class and wrapped the ProcessBlock call in a try/catch. What do you think?
Enkidu93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @ddaspit)
src/SIL.Machine/Corpora/PlaceMarkersUsfmUpdateBlockHandler.cs line 42 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
It would be more generic if we pass in the error handler to the
UpdateUsfmParserHandlerclass and wrapped theProcessBlockcall in a try/catch. What do you think?
Yes, it would. That was the other option that was at the forefront of my mind.
The benefit of passing the error handler to the specific update block handlers is that it would allow more fine-grained control. For example, there may be situations in which, for a particular handler, we want the code to crash if a block cannot be processed (e.g., if we need that update to go through before being processed by a subsequent block or if it's integral to producing the USFM). If QuotationMarkDenormalizationFirstPass were implemented as a block handler instead, we might have that sort of issue.
Since this isn't something we need right now, I'll update the code to just pass it to the UpdateUsfmParserHandler. Need be, we could move it to the individual handlers or even just pass the handler instance into the error function to provide the caller more context.
ddaspit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ddaspit reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)
Fixes sillsdev/serval#764 and fixes #332
As I began coding, I realized that the idea of 'error handler' wasn't concrete enough - there are a number of places and ways we could inject an error handler. We could also design this exception a couple different ways. This is all to say, please say 'That's not at all what I was thinking', @ddaspit, if that's the case 😆. I figured it was simpler just to put in the PR and discuss here.
This change is