[GitHub] [trafficcontrol] ocket8888 edited a comment on pull request #5162: Traffic Ops API Errors structure

[GitBox <gi...@apache.org>]

ocket8888 edited a comment 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.
   
   > If 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:
users@infra.apache.org