Reworked error output to send IETF RFC-7807 compatible problem responses #14
Conversation
Also fixed indentation
…f/wharf-provider-azuredevops into feature/use-wharf-core-ginutil
- Sending error responses has mostly been moved from runAzureDevOpsHandler to the functions that it calls instead. - The name of functions that write an error to the gin.Context has the suffix -WritesError. - Added some helpers in the form of extensions of existing code. (Not sure about this decision) - ginutilext, wharfapiext, requests It still feels really messy. I hope I didn't make it worse :&
Alexamakans
changed the title
internal/azure/importer.go
Outdated
| i := ImportData{} | ||
| err := c.ShouldBindJSON(&i) | ||
| if err != nil { | ||
| ginutil.WriteInvalidBindError(c, err, | ||
| "One or more parameters failed to parse when reading the request body for import details.") | ||
| return nil, false | ||
| } |
There was a problem hiding this comment.
With the argument of SRP, then I have some comments:
-
The new
internal/azurepackage seems to deal with:- Utility methods to talk to the Azure DevOps API in a type-safe fashion, which should be its own package
- Utility type to use the Azure DevOps API and the Wharf API and transfer data between them, which should be its own package
- Handling and validating of incoming Gin HTTP requests, which should be its own package or live inside the
mainpackage
-
The
internal/azure/importer.gofile seems to manage:- wharf-provider-azuredevops' incoming JSON structs and how they're serialized (with the
`json:"tokenId"`attributes), which should live inside themainpackage next to the Gin method that accepts that JSON body. - Azure DevOps JSON response models, which should live in the same package (or even file) as the Azure DevOps API wrapper
- wharf-provider-azuredevops' incoming JSON structs and how they're serialized (with the
Would rather see importer.go and client.go live in separate packages
…ient.go and unexported them
There was a problem hiding this comment.
Looked over this again just now and the code still looks nice to me but im having a hard time finding good answers to my usuall questions. Why did we open this PR? What problem does this solve? How do we benefit from solving it?
I can trace this back to vauge mentions of error output and error descriptions. As this PR alone now has 83 comments and its not the only PR that exsists for this goal i think its fair to demand an exlanation of why this is not simple accept&merge ~10 line string change. If this exsits please link it to the PR and in the changelog's change description.
There was a problem hiding this comment.
typo: Title for 21ff73b is supposed to be Renamed importData -> importBody.
Closes #13
Depends on iver-wharf/wharf-core#17