itchyny / gojq

Pure Go implementation of jq

MIT License

3.3k stars 119 forks source link

code review #249

Closed ccoVeille closed 7 months ago

ccoVeille commented 7 months ago

remove useless receivers
remove useless else statement
avoid wrapping issues

ccoVeille commented 7 months ago

I started looking at the code and I found bad practice that could lead to panic or error if the code is refactored

itchyny commented 7 months ago

This is meaningless change applying unthoughtful best practices. The errors are controlled in the package and errors.Is is reasonable only when the error comes from another package, also I don't like for performance reasons. Return statement in else is intentional, not to catch unexpected failure of forgetting return in previous branches, possibly caused by nested branches. The first commit about useless receivers is the only one I can merge in the changes.

ccoVeille commented 7 months ago

I get your points. I disagree with some of them, but OK, I don't mind.

I'll drop the commits you find useless.

ccoVeille commented 7 months ago

I will need time and review from you to make sure the place you want me to use errors.Is/errors.As

I'm splitting this PR in two then.

250

251

I'm dropping the refactoring on switch/else.