[#605] Use more appropriate field names #709
Conversation
elliott-beach
changed the title
We can merge them soon one after another, but I don't think it should be a goal to unify unrelated breaking API changes just because they both break the API. In fact, it's better not to, because then people can upgrade one version at a time if they wish. Also, it makes reverting one of them easier, if necessary (not that reverting is likely to happen, but there's always a non-zero chance). They're separate and independent changes, let's treat them that way. |
github/repos_deployments.go
Outdated
| @@ -137,7 +137,7 @@ type DeploymentStatus struct { | |||
| Description *string `json:"description,omitempty"` | |||
| TargetURL *string `json:"target_url,omitempty"` | |||
| CreatedAt *Timestamp `json:"created_at,omitempty"` | |||
| UpdatedAt *Timestamp `json:"pushed_at,omitempty"` | |||
| PushedAt *Timestamp `json:"pushed_at,omitempty"` | |||
There was a problem hiding this comment.
Now that I've looked, are we sure pushed_at is the right JSON field?
It doesn't look to be. I think it might be updated_at instead.
Take a look at https://developer.github.com/v3/repos/deployments/. There are 7 hits for "updated_at", 0 for "pushed_at".
elliott-beach
force-pushed
the
rename-fields
branch
from
fdcaacc to
7f746a5
Compare
|
@shurcooL This should be good-to-go now. |
|
Thanks again @e-beach. |
…ogle#709) Authorization struct had a "updated_at" JSON key mapped to UpdateAt, which was an oversight/typo. It has been renamed to UpdatedAt, like all other similar fields. Deployments and DeploymentStatus structs had incorrectly mapped their UpdatedAt field to a non-existing "pushed_at" JSON key. That means their values were never populated. This change fixes that. Bump library version as this is a breaking change. Helps google#605.
This PR renames two fields as per discussion in #605:
Authorizations.UpdateAt=>UpdatedAtDeploymentStatus.UpdatedAt=>PushedAtAs this is a breaking API change, would it be appropriate to merge these changes as a single version bump with #706, also a breaking change?