Review matthieu #4
Conversation
This reverts commit f9d8a75.
| // MakeBypass uses a make to bypass the Smartcar Connect brand selector. | ||
| // Smartcar Pro feature. | ||
| type MakeBypass struct { | ||
| Make string |
There was a problem hiding this comment.
Consider introducing types to avoid having "stringly-typed" signatures:
type Make string
type Scope string
// ...This makes call-sites more explicit and less error-prone, especially when multiple such types are used together.
When the number of valid values is small (e.g. for UnitSystem), it is also common to expose the values themselves and use a different underlying type for additional safety:
type UnitSystem int
const (
Metric UnitSystem = iota
Imperial
)However one downside of a non-string-based type is that serialization becomes less straightforward.
| @@ -0,0 +1,20 @@ | |||
| package constants | |||
There was a problem hiding this comment.
I'm not very familiar with open-source Go but it doesn't look like using many nested packages is common. To keep things lightweight, I would recommend keeping the hierarchy as flat as possible, possibly even in the same folder (from taking a quick look at popular packages, many have 20+ files in a given folder, e.g. pg).
Additionally, I would recommend introducing internal packages only if they have a thinner public API than their implementation. When this isn't the case (e.g. here, where everything is public), you don't benefit from the package boundary. Having too many distinct packages has a cost on maintenance.
| func Request(method, url, authorization, unitSystem string, body io.Reader) (http.Response, error) { | ||
|
|
||
| // Build Request | ||
| req, reqErr := http.NewRequest(method, url, body) |
There was a problem hiding this comment.
It's good practice for non-trivial API methods (e.g. those which make HTTP requests, database queries, remote calls, ...) to take in a context.Context as first argument. This allows the client to cleanly control cancellations, timeouts, etc. I strongly recommend following this pattern in the SDK. For this particular call, take a look at http.NewRequestWithContext.
| ) | ||
|
|
||
| // GetUserID returns the user ID of the vehicle owner associated with the accessToken. | ||
| func GetUserID(accessToken string) (string, error) { |
There was a problem hiding this comment.
Several methods have a Get prefix (GetUserID, GetVehicleIds, ...), others don't (vehicle.Info, vehicle.VIN`). Can you make them consistent?
| ``` | ||
| 5. Construct vehicle | ||
| ```go | ||
| vehicle := smartcar.Vehicle{ID: vehicleIds[0], AccessToken: token.Access} |
There was a problem hiding this comment.
I would recommend using a constructor for this, so that you can keep the ID field internal and immutable:
func NewVehicle(id, token string) *VehicleOtherwise you should document exactly what happens when the ID is mutated (e.g. concurrently with requests) or that it isn't allowed (but then you might as well guarantee it via the type system).
| ``` | ||
| 6. Send request to vehicle | ||
| ```go | ||
| // Vehicle Endpoints |
There was a problem hiding this comment.
API suggestion in case you have a batch endpoint (or plan on having one in the future) for "getter" methods:
type Key int
const (
Info Key = iota
VIN
// ...
)
type Data struct {
Info *Info // Only populated if the Info key was present
VIN int // Only populated if the VIN key was present
// ...
}
func (v *Vehicle) Data(keys ...Key) (*Data, error)| ```go | ||
| vehicle := smartcar.Vehicle{ID: vehicleIds[0], AccessToken: token.Access} | ||
| ``` | ||
| 6. Send request to vehicle |
There was a problem hiding this comment.
I might have missed it but I didn't see any specific handling below for expired tokens. As an API user, I would likely want to handle the case of an expired token error differently than others (e.g. to trigger a refresh). One simple option to enable this (without pattern matching on error strings) is to introduce a variable for this:
var ErrExpiredToken = errors.New("expired token")Alternatively you could introduce a custom type, but that might not be worth it unless you want to add functionality (e.g. formatting or convenience for refreshing the token).
In any case, you should document what happens here :).
|
Closing PR because it was only created to be reviewed by matthieu |
No description provided.