code review #249
code review #249
Conversation
ccoVeille
commented
Apr 4, 2024
ccoVeille
commented
- remove useless receivers
- remove useless else statement
- avoid wrapping issues
leaving as soon as possible avoid complexity
- errors should be checked with errors.Is - errors should be compared with errors.As - errors should be wrapped with %w in fmt.Errorf
|
I started looking at the code and I found bad practice that could lead to panic or error if the code is refactored |
|
This is meaningless change applying unthoughtful best practices. The errors are controlled in the package and |
|
I get your points. I disagree with some of them, but OK, I don't mind. I'll drop the commits you find useless. |
| var syntaxErr *json.SyntaxError | ||
| if errors.As(err, &syntaxErr) { | ||
| syntaxErr.Offset -= i.offset | ||
| offset, line = &syntaxErr.Offset, &i.line |
There was a problem hiding this comment.
This part is good, the error comes from json package.
| var syntaxErr *json.SyntaxError | ||
| if errors.As(err, &syntaxErr) { | ||
| syntaxErr.Offset -= i.offset | ||
| offset, line = &syntaxErr.Offset, &i.line |
| @@ -2065,7 +2068,7 @@ func compileRegexp(re, flags string) (*regexp.Regexp, error) { | |||
| } | |||
| r, err := regexp.Compile(re) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("invalid regular expression %q: %s", re, err) | |||
| return nil, fmt.Errorf("invalid regular expression %q: %w", re, err) | |||
There was a problem hiding this comment.
This change is fine, but I don't think gojq library users unwrap the execution errors though.
| @@ -98,7 +99,7 @@ func (l *moduleLoader) LoadJSONWithMeta(name string, meta map[string]any) (any, | |||
| for { | |||
| var val any | |||
| if err := dec.Decode(&val); err != nil { | |||
| if err == io.EOF { | |||
| if errors.Is(err, io.EOF) { | |||
