Skip to content

Continue parsing file when there's a conversion error#224

Merged
mganss merged 4 commits intomganss:masterfrom
PauloTDK:EventAndReportInsteadException
Nov 17, 2022
Merged

Continue parsing file when there's a conversion error#224
mganss merged 4 commits intomganss:masterfrom
PauloTDK:EventAndReportInsteadException

Conversation

@PauloTDK
Copy link
Copy Markdown
Contributor

@PauloTDK PauloTDK commented Nov 16, 2022

Problem being addressed:

  • Given that a reasonable Excel file can have multiple unconvertible columns to the intended entity;
    • Should be possible to list convertion errors without having to edit source file and restart the process;
      • So it will be possible to create a "quality" report;
      • So it will be possible correct the file in a single task, or if interesting, work those entities manually

Description of solution:

  • Created an event to notify the error, that will be triggered only if configured to do so (parameter), otherwise the exception will be thrown as usual.
  • Created a specific file containing multiple rows with multiple errors.

… to target type fails.

Instead, an event will be triggered, and, through this event a report can be made, with all the conversion errors caught.
To this intent, an specific file containing multiple rows with errors.
Also, bumped the .net framework, since version 5 is already out of support.
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 16, 2022

Codecov Report

Base: 93.71% // Head: 93.56% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (54938b8) compared to base (b86a0e0).
Patch coverage: 93.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   93.71%   93.56%   -0.16%     
==========================================
  Files          10       11       +1     
  Lines        1336     1351      +15     
  Branches      195      197       +2     
==========================================
+ Hits         1252     1264      +12     
- Misses         54       57       +3     
  Partials       30       30              
Impacted Files Coverage Δ
ExcelMapper/ExcelMapper.cs 94.27% <90.00%> (-0.30%) ⬇️
ExcelMapper/ParsingErrorEventArgs.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Increase code coverage (trial1)
@mganss
Copy link
Copy Markdown
Owner

mganss commented Nov 16, 2022

I like this idea. OTOH there's a .NET design guideline that says:

❌ DO NOT have public members that can either throw or not based on some option.

Yet I think it's definitely a legitimate use case. So I'm leaning towards accepting this regardless. Or can you perhaps think of a different way to enable this use case?

Perhaps we could always report errors through an event with args derived from CancelEventArgs and throw if Cancel is not set to true by the handler.

@PauloTDK
Copy link
Copy Markdown
Contributor Author

Hi!
Glad that this idea has echoed!
I've ended up using your sugestion, since it added versatility, making also possible to check if the error is "acceptable" at runtime.

@mganss mganss merged commit d732697 into mganss:master Nov 17, 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