ocket8888 commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710634613
> ... we would only ever want to populate either `SystemError` or
`UserError`, never both
That sounds pretty reasonable, but there are multiple cases right now where
both are explicitly populated.
> It smells funny to return a non-nil Errors struct in both the success and
errors cases -- it goes against the Go idiom of "nil error means success".
Instead of checking for nil, you have to call a special method on the return
value
Well it's not actually an `error`, it just contains them. The `error`s it
contains actually don't go against that idiom, and you can do the check
yourself if you want; you don't "have" to call a special method on the return
value. It just seemed easier and cleaner.
> I don't really like creating the Errors struct at the beginning of the
method then just setting its fields before returning it. If possible, I think
errors should be declared in the same line they're returned. If the entire
function/method has access to the error struct, it can be modified in
unexpected ways.
There are definitely many places where it could just return a struct
literal. In most cases I don't really have a preference for one over the other,
but there might be a few places where I'd want to try to make a case for
modifying values.
To conclude: I don't really have a problem with reworking this to be a
single `error` if people like that solution better. It does seem more elegant,
but the problem is that would entail changing logic, because there are places
that return both errors, and I don't think that should be done without
examining those use-cases.
> A single error interface seems more idiomatic-Go. But it's not really very
useful, if at all. Because you always need to check and handle both cases.
No - they do need to both be checked, but _I_ don't need to do it. That's
what `api.HandleErrs` is for, it can all be checked in one place so I don't
need to do a switch everywhere, or check two values every time. I really do
think collapsing that would be an improvement.
> f the common helper funcs just return an error, what does a helper func
that handles it do when it gets an error that isn't a UserAndSystemError
That would ideally impossible. I think, instead of using types to represent
the concept as Rawlin suggested - because you're right, Go doesn't do those
complex types very well - we could instead store that information in the
structure itself:
```go
type Err struct {
RawError error
Code int
UserSafe bool
}
```
>That was my mistake, and I apologize. IMO the idiom should always be
"userErr, sysErr, errCode". The funcs that got it backward are my fault, and
should be fixed.
Are you sure? The Go standard library always returns errors last in
multi-value returns, and go-lint's default ruleset would have a problem with
all of our call signatures if they didn't conform to that convention.
Then helpers just return that instead of `error`, so we maintain type-safety.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
