-
Notifications
You must be signed in to change notification settings - Fork 53
Hydration with Href, ID, and Get/Reload abstraction #280
base: master
Are you sure you want to change the base?
Conversation
organization := new(Organization) | ||
|
||
resp, err := s.client.DoRequest("GET", apiPathQuery, nil, organization) | ||
href := path.Join(organizationBasePath, organizationID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we implement a GetBasePath() string
for *ServiceOp
, and NewResource() interface{} // return new(Organization)
, we could implement Get
in a ServiceOp type embedded in {Every}ServiceOp
.
We would get Get
and Hydrate
for free (using the ServiceOp.Get implementation) in each endpoint (that uses Href and ID).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could get Delete and others for free?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cool to have Get method for free. Just rarely there are still some resources with more than one ID param to lookup. not fitting the Get(string, *GetOptions)
signature. Simple command grep -P "^\tGet\(" *go | grep string
shows me only vlan assigments are the case at the moment (because I have the #302 PR checked out). But I think API keys are special too.
Then, I'm not sure how/where we'd ensure that the Get method returns the desired resource type. Right now the type is declared in the Get signature Get(DeviceID string, opts *GetOptions) (>>*Device<<, *Response, error)
.
So I think, for the parent interface would have to do the Hydrate method as you wrote it, and for every concrete ServiceOp we would have to implement:
func GetResourcePath(id string) string { return path.Join(deviceBasePath, id) }
func DefaultIncludes() []string { return []string{"facility", "hardware_reservation", "project"} }
(this could be an attribute)func NewResource() *Device { return new(Device) }
- Get like
// this could be copy-pasted or template-generated to all the service opts func (s *DeviceServiceOp) Get(id string, opts *GetOptions) (*Device, *Response, error) { resource := s.NewResource(s.GetResourcePath(id)) opts = opts.Including(s.DefaultIncludes()) resp, err := s.Hydrate(resource, opts) if err != nil { resource = nil } return resource, resp, err }
devices.go
Outdated
@@ -364,15 +366,33 @@ func (s *DeviceServiceOp) List(projectID string, opts *ListOptions) (devices []D | |||
} | |||
} | |||
|
|||
func (d *Device) GetHref() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not needed if Device embeds *Href
(https://github.com/packethost/packngo/pull/280/files#diff-ebde46360e5c028cc3bde9d49c49bfb6038d13cd340af1d2d61845323ec284fbR59-R64), as this PR does with Organization https://github.com/packethost/packngo/pull/280/files#diff-d5c3dbfdac6c7539c18385ddd9c9dceff98dfda42c0bc25acaf87135caf6e066R31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do the *Href embedding, it seems like a huge save on amount of code. I don't think it will break too many things as the Href field is not used so much AFAIK.
Signed-off-by: Marques Johansson <mjohansson@equinix.com>
Signed-off-by: Marques Johansson <mjohansson@equinix.com>
WIP on #269