chore: introduce encoding factory #3740
Conversation
achettyiitr
force-pushed
the
chore.encoding-init
branch
from
f84572f
to
dc6bea1
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3740 +/- ##
==========================================
- Coverage 68.78% 68.73% -0.06%
==========================================
Files 345 344 -1
Lines 51917 51936 +19
==========================================
- Hits 35711 35698 -13
- Misses 13907 13941 +34
+ Partials 2299 2297 -2
☔ View full report in Codecov by Sentry. |
achettyiitr
force-pushed
the
chore.encoding-init
branch
3 times, most recently
from
54744d4
to
c7d6bf7
Compare
warehouse/encoding/encoding.go
Outdated
| type LoadFileWriter interface { | ||
| type Writer interface { |
There was a problem hiding this comment.
I find the old name a bit more specific actually. Writer is a very generic name but this interface has the GetLoadFile for example that makes it really specific.
There was a problem hiding this comment.
Make sense. Reverted it.
There was a problem hiding this comment.
I was thinking in terms of the warehouse module only where:
- Readers -> Load file Readers.
- Writers -> Load file singular event writers.
- Loaders -> Load file singular event loaders.
warehouse/encoding/encoding.go
Outdated
| } | ||
| } | ||
|
|
||
| type Loader interface { |
There was a problem hiding this comment.
I wonder if we could came up with a better name for this one. It lives in the warehouse/encoding package so if we just call it Loader it doesn't tell us much about what it is supposed to load and for sure it doesn't look like we can use it to load "everything", right?
As a sidenote, it doesn't look very SOLID. I'm not asking for changes in this regard but I think it's beneficial to mention as a part of a broader discussion to gather your feedback. For example in SOLID the S stands for "Single Responsibility". When looking at this interface though we see that whoever is going to implement this interface will have multiple responsibilities (e.g. parsing in order to understand if a column name is a load time column, ability to add columns, ability to write data somewhere, etc...).
An initial step towards a more SOLID approach, we could split this interface into multiple ones. Luckily with Go the same struct can implicitly implement multiple interfaces so we won't have to create more structs as a result, at least at the beginning. Still having more than one interface here can help grasp the code and can help with understanding the dependencies and responsibilities of these components. Usually this is going to help with tests and mocks as well. I hope it makes sense, feel free to share your thoughts.
There was a problem hiding this comment.
Yeah, I completely aggress with having a solid interface.
fracasula
changed the title
fracasula
changed the title
| return []string{}, fmt.Errorf("scanner scan: %w", err) | ||
| } | ||
| return []string{}, io.EOF | ||
| } | ||
|
|
||
| lineBytes := js.scanner.Bytes() | ||
| jsonData := make(map[string]string) | ||
| jsonData := make(map[string]interface{}) |
There was a problem hiding this comment.
This came across while writing the tests. It's expecting only string which will not be the case.
There was a problem hiding this comment.
We are doing cast.ToStringE(jsonData[columnName]) to load into the record.
achettyiitr
force-pushed
the
chore.encoding-init
branch
from
c7d6bf7
to
a187745
Compare
achettyiitr
force-pushed
the
chore.encoding-init
branch
from
eef8f8a
to
6d3c84b
Compare
achettyiitr
force-pushed
the
chore.encoding-init
branch
from
6d3c84b
to
b876871
Compare
achettyiitr
force-pushed
the
chore.encoding-init
branch
from
9182d5d
to
9412809
Compare
achettyiitr
changed the title
achettyiitr
force-pushed
the
chore.encoding-init
branch
from
9412809
to
e5c4032
Compare
Description
encoding.Init().Linear Ticket
Security