feat: Domains API operations #208
Conversation
gkats
force-pushed
the
core-1512-domains
branch
from
2a2f9b5 to
89cbad2
Compare
| @@ -258,7 +258,7 @@ func (b *defaultBackend) do(req *http.Request, params Queryable, setter Response | |||
| func setRequestBody(req *http.Request, params Queryable) error { | |||
| // GET requests don't have a body, but we will pass the params | |||
| // in the query string. | |||
| if req.Method == http.MethodGet { | |||
| if req.Method == http.MethodGet && params != nil { | |||
| q := req.URL.Query() | |||
| params.Add(q) | |||
There was a problem hiding this comment.
❓ I just noticed this and I'm not sure I understand it 100%. This also allows for some room for someone to erroneously tamper with the q object, if they implement their own custom struct, isn't that right? I would probably expect it to be the other way round, but let me know what you think (and this goes up to the APIRequest struct I believe):
func setRequestBody(req *http.Request, params url.Values) error {
if req.Method == http.MethodGet && params != nil {
q := req.URL.Query()
for key, values := range params {
for _, v := range values {
q.Add(key, v)
}
}The Queryable interface could be then:
type Queryable interface {
ToQueryParameters() url.Values
}There was a problem hiding this comment.
Great point! I'll update the interface and the logic. I'd rather do it on a separate PR. Do you think that would be a problem?
There was a problem hiding this comment.
Not a problem at all, by all means proceed with merging 🚀
| @@ -0,0 +1,25 @@ | |||
| package clerk | |||
There was a problem hiding this comment.
❓ Couldn't we use the domain package here instead?
There was a problem hiding this comment.
We could, but I'm afraid that at some point we might need this type from another package. Imagine an Instance which has a nested Domain.
I believe it's safer to have these types for "models" in the clerk package so we don't run into any circular dependecy problem in the future.
There was a problem hiding this comment.
I see, I thought that we might want to reuse structs. Do you think that if we introduced a models/<service> kind of structure, we might run into circular dependency problems? Just thinking out loud here:
import (
domainmodels "github.com/clerk/clerk-sdk-go/v2/models/domains"
)There was a problem hiding this comment.
Thanks for the suggestion! Your solution would certainly avoid circular dependency, but isn't it a bit too much packaging?
Is there any reason why using the clerk package might be problematic?
There was a problem hiding this comment.
It's not problematic, it's only that packages provide a native namespace and the import aliases provide some more naming flexibility for someone that uses them. Names also become more uniform.
package domain
// Debatable name :)
type Object struct{}
type List struct {
Data []Object `json:"data"`
}import (
domainmodels "github.com/clerk/clerk-sdk-go/v2/models/domains"
)
func HandleDomains() {
var domainList domainmodels.List
//...
}Just an idea here, not insisting on this, it might be overly verbose/complicated for some reason. Noting now, since it would be a breaking change afterwards!
| } | ||
|
|
||
| // List returns a list of domains | ||
| func List(ctx context.Context, params *ListParams) (*clerk.DomainList, error) { |
There was a problem hiding this comment.
🔧 If there's not a strong reason to use pointers, I think we should avoid them where possible, as they make garbage collection more difficult and the risk of accidental mutations increases.
| } | ||
|
|
||
| func (rt *mockRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { | ||
| if rt.status == 0 { |
There was a problem hiding this comment.
💡 Mostly a suggestion, it might be easier to use a constructor for initializations: newMockRoundTripper
| clerk.SetBackend(clerk.NewBackend(&clerk.BackendConfig{ | ||
| HTTPClient: &http.Client{ | ||
| Transport: &mockRoundTripper{ | ||
| T: t, |
There was a problem hiding this comment.
🔧 Don't you want to assert that the GET method is used here?
| HTTPClient: &http.Client{ | ||
| Transport: &mockRoundTripper{ | ||
| T: t, | ||
| out: json.RawMessage(`{ |
There was a problem hiding this comment.
💡 You can also introduce a helper here:
out: MustJSONMarshal(clerk.DomainList{}),Since JSON serialization/deserialization is generally symmetric, I think this doesn't break the expectation validity here.
Added types for Domains. Added a new package for Domains API operations. Supports List, Create, Update and Delete.
gkats
force-pushed
the
core-1512-domains
branch
from
89cbad2 to
1aad70d
Compare
Type of change
Description
Added types for Domains.
Added a new package for Domains API operations. Supports List, Create, Update and Delete.