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

Add backwards-compatible fix to make handle optional #143

Merged
1 commit merged into from
Mar 5, 2018

Conversation

PauloMigAlmeida
Copy link
Contributor

What is the problem?

According to Datadog's API Reference, REST methods responsible for dealing with the Comment entity can have the Handle property optionally set which in some cases, this can be rather handy. It turns out that the way the API is currently set forces the developer always to specify it regardless of the omitempty option in the Comment struct

Why omitempty option fails during the json marshall phase?

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

Although the piece of documentation above lead us to believe that if we set the handle argument to an empty string then it would work by itself, there is a caveat due to the Go lang String(string) helper function which creates a pointer for that empty string and subsequently, the criteria the omitempty no longer applies as shown in this other piece of documentation below:

Pointer values encode as the value pointed to. A nil pointer encodes as the null JSON value.

What has been done to fix the problem?

There are a few solutions but it will all depend upon when you guys are planning to release the next major version of this framework. The solution could be simple by changing the related functions' arguments from built in string type to a string*. This alone (along with the changes for making this compilable) would be sufficient to make it work as you can always set a string* to nil and the json marshall process wouldn't send it to the the API.

Example:

// CreateComment adds a new comment to the system.
func (client *Client) CreateComment(handle *string, message string) (*Comment, error) {
	var out reqComment
	comment := Comment{Handle: handle, Message: String(message)}

	if err := client.doJsonRequest("POST", "/v1/comments", &comment, &out); err != nil {
		return nil, err
	}
	return out.Comment, nil
}

However, this would break backwards-compatibility due to the change of methods' signatures and because of that, I picked a less invasive solution which gives you the control of when to tackle this design improvement at your own pace.

Having said that, the solution I proposed in this PR was simply checking whether or not the handle argument is empty and in case it isn't, then the program will set Handle property in the Comment struct to that value.

Best regards,

Paulo Almeida

@ghost ghost merged commit 0e5b3cf into zorkian:master Mar 5, 2018
@ghost
Copy link

ghost commented Mar 5, 2018

Thanks for this!

This pull request was closed.
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.

1 participant