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

removing omitempty by customer request #49

Merged
merged 3 commits into from
Jun 12, 2018
Merged

Conversation

alistanis
Copy link
Contributor

By customer request, we're removing omitempty so that customers can remove filters applied to a record by sending either [] or null - @jamorency

Implications are that if customers want to update a record, they must always send the full set of filters, or they will not be applied. We've spoken about other possible breaking changes and have decided that this is still worth pursuing.

@alistanis alistanis requested a review from pashap June 12, 2018 16:02
Copy link
Member

@fcelda fcelda left a comment

Choose a reason for hiding this comment

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

LGTM. The underscores in example_test.go are intentional?

Copy link

@larsx2 larsx2 left a comment

Choose a reason for hiding this comment

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

Looks just fine. What is the reason of the convention change?

@alistanis
Copy link
Contributor Author

alistanis commented Jun 12, 2018

@fcelda Yeah, interestingly when you're writing a function to test a method on a type, underscores are the idiomatic way of doing that. Travis recently updated their tests to fail example/test code that doesn't match identifiers consistent with that. @larsx2 In regard to omitempty, customers want to be able to remove filters, and to do that with the api, we need to send "filters": [] or "filters": null

@alistanis alistanis merged commit 4a57e76 into v2 Jun 12, 2018
mioi added a commit to mioi/ns1-go that referenced this pull request Jun 29, 2018
after PR ns1#49 was merged, there was an issue creating new records.
this should fix it.
mioi added a commit to mioi/ns1-go that referenced this pull request Jun 29, 2018
after PR ns1#49 was merged, there was an issue creating new records.
this should fix it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants