FS-857 Add parse XLSX file function for BOM service with unit tests covered #235
Conversation
Alva8756
changed the title
Alva8756
force-pushed
the
FS-857
branch
10 times, most recently
from
131229f
to
a017fb3
Compare
…box#233) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #235 +/- ##
==========================================
+ Coverage 71.87% 72.40% +0.52%
==========================================
Files 36 37 +1
Lines 3549 3627 +78
==========================================
+ Hits 2551 2626 +75
- Misses 745 747 +2
- Partials 253 254 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Alva8756
changed the title
There was a problem hiding this comment.
This is an impressive bit of work. I left a few comments that I'd like you to address, but nothing really show-stopping. Nice job!
This is marked as Needs Changes because it shouldn't be in ServerService (as we discussed earlier). But I think you could lift-and-shift this code to a proxy service and it would be fine.
There was a problem hiding this comment.
Consider putting all the structs into a single file.
| // which is the format of a file reading from the HTTP request. | ||
| stat, err := file.Stat() | ||
| if err != nil { | ||
| fmt.Println(err) |
There was a problem hiding this comment.
you have a couple of better (IMO) options here. Consider t.Logf, t.Fatalf (better, I think) or require.NoError (if you are familiar with github.com/stretchr/testify/require.
BOM service PostBomXlsx API need a parse XLSX helper function.
Implementation for XLSX parse helper function.
wrote unit tests cover for the helper function.
testdata folder to hold all xlsx files for testing purpose.