Conversation
tommyblue
left a comment
tommyblue
left a comment
There was a problem hiding this comment.
lgtm, just minor comments
| Fields []string `help:"list of fields to match with the regular expression in Regexs" default:"[]"` | ||
| Regexs []string `help:"list of regular expression to match. Fields[0] must match Regexs[0], Fields[1] Regexs[1] and so on" default:"[]"` |
There was a problem hiding this comment.
do you think it's acceptable to have those empty defaults? I mean, it doesn't hurt but also does nothing. I think we could remove defaults and require at least 1 field (and then 1 regexp)
There was a problem hiding this comment.
not handling is also less code to write/read/maintain and potential bugs. I think it's acceptable to do nothing in this case but I don't have a very strong opinion on this.
I just started to work on the required fields.
When it will land, it will be easy to remove the defaults and set those fields as required so as to have Baker directly handles these kinds of cases
| }, | ||
| } | ||
|
|
||
| fieldByName := func(name string) (baker.FieldIndex, bool) { |
There was a problem hiding this comment.
just to clarify the reading of the test table (in particular the correspondance between fields and record values), you could call the fields something like "1st", "2nd", "3rd". And then also "foox" could be "unexistentField" to be immediately clear why that test has wantErr = true
There was a problem hiding this comment.
Rename foox into non-existent is a good idea but I think the foo bar baz idiom is well-known enough when names do not import. There's also the fieldByName function in the middle of the test that clearly shows what they are.
Co-authored-by: Tommaso Visconti <tommaso.visconti@adroll.com>
Co-authored-by: Tommaso Visconti <tommaso.visconti@adroll.com>
2 participants
❓ What
Add RegexMatch filter: discards records if one or more of the specified fields do not match a specified regular expression.
🔨 How to test
✅ Checklists
This section contains a list of checklists for common uses, please delete the checklists that are useless for your current use case (or add another checklist if your use case isn't covered yet).
make gofmt-writebeen run on the code?make govetbeen run on the code? Has the code been fixed accordingly to the output?