Structs should use pointers for fields #29
Comments
|
I think I agree, but want to think about it for another couple of days before we start changing stuff. And when we do start changing this, I prefer to change it with one PR for the whole package. This way everything stays consistent and we don't have two different patterns at the same time. So I'll come back on this one, but like I said I think I agree 😀 |
|
@martinsefcik after thinking about this for a little while I agree that we would need to change this. The problem is of course that a change like this would be a breaking change, but I think there is no way to get around that... So let's go ahead and (when time permits) start the work on this one 😀 Since it's a breaking change I do prefer one big PR for the whole change and some minimal helper functions that can be used by users of this package to similar to what AWS has done https://github.com/aws/aws-sdk-go/blob/master/aws/convert_types.go With that's in place I think the burden of updating code does not outweigh the benefits from having a better API on which we can continue to build for the future. |
|
If I will have some free time I can start working on it. |
* Add visibility_level parameter to CreateGroupOptions * Changed Visibility parameter type to VisibilityLevel * Changed Visibility paramater name to VisibilityLevel to match existing fields' names * Changed VisibilityLevel parameter to pointer to correctly omit empty (nil) value. See #29
…README and project struct (#66) * Convert all *Options structs to use pointer for fields, per #29. * Update README and fix project struct The `project` struct was the only one that had all pointers, so I reverted that to make things consistent again. The same goes for the `time.Time` pointers. In some cases the API does not return a time and if the field is not a pointer to `time.Time`, this will generate an error. So to be consistent now all `time.Time` fields are pointer fields.
|
Fixed by PR #66 |
|
👍 |
|
This is a rather surprising change, mostly because the API now seems very verbose and repetitive for no reason. Then again, I can't think of a better solution and it seems like |
|
@mvdan I understand your point and I partly agree... Yet I don't agree it is "for no reason" as certain use cases do not work without this change... And yes, I too cannot think of a better/other way so following the |
I meant to say "at first glance" :) It seems like a non-idiomatic way to do things, but as said it seems like the only way. |
|
Agree... Not the most idiomatic, yet there are quite a couple of packages using such an approach (mostly API client packages of course). See for example this blog from 2 years ago already: https://willnorris.com/2014/05/go-rest-apis-and-pointers |
This adds the page query parameter to the URL call to the API when fetching projects or issues. If the value exceeds the total number of pages available no results are returned. Co-Authored-By: Max Jonas Werner <makkes@users.noreply.github.com>
I think we should change fields in structs to use pointers and so distinguish between nil and empty values.
The same approach is used in go-github repository.
Here you can find detailed blog why it is good to use pointers:
https://willnorris.com/2014/05/go-rest-apis-and-pointers
It's also problem in go-gitlab.
For example you cannot edit existing project's description and change it to empty string, because empty description value will never be sent to server (will be omitted).
What do you think ?
The text was updated successfully, but these errors were encountered: