search

sillsdev / l10nsharp

A .NET localization library for Windows Forms applications.
3 stars 10 forks source link

Replace exception rethrow with writing warning message (20221006) #105

Closed StephenMcConnel closed 1 year ago

StephenMcConnel commented 1 year ago

This change is Reviewable

github-actions[bot] commented 1 year ago

Test Results

    2 files  ±0    44 suites  ±0   12s :stopwatch: +3s 148 tests ±0  143 :heavy_check_mark: ±0  5 :zzz: ±0  0 :x: ±0  296 runs  ±0  288 :heavy_check_mark: ±0  8 :zzz: ±0  0 :x: ±0 

Results for commit 900d199c. ± Comparison against base commit e0c7bd6b.

:recycle: This comment has been updated with latest results.

tombogle commented 1 year ago

src/L10NSharp/CodeReader/StringExtractor.cs line 115 at r1 (raw file):

                  else
                  {
                      // Rethrowing the exception just ends up crashing the calling program, which is way overkill.

I see the point of this, but I wonder if we have done enough to try to figure out what specific scenarios can cause this and analyzed them to determine if something more appropriate can be done? I see there are three non-test callers of this method. If some of them could perhaps benefit from getting back a list of errors that could be logged or reported in a more helpful way, that might be worth considering. An overload of this method could make that possible. If there is a known way to force one of these unexpected exceptions in a test, it might also be a good idea to write a test case that shows that the exception is eaten (or reported back, if we go that route).