Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dereference to pointer for include and query fields for ListOption structs #309

Merged
merged 1 commit into from Feb 14, 2022

Conversation

sebasslash
Copy link
Contributor

@sebasslash sebasslash commented Feb 14, 2022

Description

The PR title says it all. This stems from an effort to make all list option structs pointers. Passing omitempty to the jsonapi serializer takes care of ignoring unset list options.

@sebasslash sebasslash requested a review from a team as a code owner Feb 14, 2022
@sebasslash sebasslash requested a review from uturunku1 Feb 14, 2022
@sebasslash sebasslash force-pushed the sebasslash/remove-list-opt-field-ptrs branch 2 times, most recently from ad282a3 to 14c555a Compare Feb 14, 2022
@sebasslash sebasslash force-pushed the sebasslash/remove-list-opt-field-ptrs branch from 14c555a to 9897eb0 Compare Feb 14, 2022
Copy link
Contributor

@uturunku1 uturunku1 left a comment

I love when changes are as clean as these ones!
I left a comment if you want to take a look at it, but I am not requesting any changes. Feel free to merge after you are done reading it :)

@@ -116,7 +116,7 @@ func (s *adminRuns) ForceCancel(ctx context.Context, runID string, options Admin

// Check that the field RunStatus has a valid string value
func (o AdminRunsListOptions) valid() error {
Copy link
Contributor

@uturunku1 uturunku1 Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this into *AdminRunsListOptions?

Copy link
Contributor

@uturunku1 uturunku1 Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM. I was looking at other listOptions that call the valid() helper function (for example func (o StateVersionListOptions) valid() ) and none of them have the pointer type in the receiver method. I guess we could add them to all or just skip this step. It doesn't seem to make any difference to have it or to not have because at the end we are not checking if listOptions is valid but only if its fields are valid.

Copy link
Contributor Author

@sebasslash sebasslash Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most important thing here is consistency and I'm fine leaving it as is. Since valid() doesn't mutate values or ever really accept a complex struct (except maybe in workspace.go) I think a value receiver makes more sense.

Here's some nice reading: https://github.com/golang/go/wiki/CodeReviewComments#receiver-type

@sebasslash sebasslash merged commit 39042a2 into releases-1.0.x Feb 14, 2022
4 checks passed
@sebasslash sebasslash deleted the sebasslash/remove-list-opt-field-ptrs branch Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants