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

Move back Precision to string type #193

Closed
wants to merge 1 commit into from
Closed

Conversation

masci
Copy link

@masci masci commented Nov 13, 2018

#179 Changed the type of Precision fields for various struct from *string to *json.Number but I can't make it work due to the fact that "*" is a valid value, meaning "full precision". Please notice acceptance tests don't expose the issue since Precision field for Widget (the one used in the tests) is still a string:

Precision *string `json:"precision,omitempty"`

Not sure what was the rationale around #179 since it mentions the API might return int for precision sometimes but if that's the case we should fix the issue on the Datadog side, normalizing all the things to strings.

This is blocking an important fix for https://github.com/terraform-providers/terraform-provider-datadog/issues/117

@nyanshak
Copy link
Collaborator

I made a small playground link to demonstrate why it should be *json.Number rather than string: https://play.golang.org/p/6TAkg8WrJsg.

Namely, Datadog can return "precision": 1 (int) rather than "precision": "1" (string), which will cause errors with *string, but not *json.Number.

For that reason, I'm rejecting this change.

@nyanshak nyanshak closed this Nov 13, 2018
@masci
Copy link
Author

masci commented Nov 13, 2018

@nyanshak try to add Precision: datadog.JsonNumber(json.Number("*")), to any TileDef in TestWidgets to expose the problem. Currently the integration tests don't exercise changes from #179.

Can you please keep this open for discussion, or should I move this to an issue? The current state of the client is half broken (in any case,

Precision *string `json:"precision,omitempty"`
is still a *string).

@nyanshak nyanshak reopened this Nov 13, 2018
@nyanshak
Copy link
Collaborator

I think you're right that Precision field in Widget being a different type than in TileDef isn't quite right. I have raised #194 to address it. Could you provide feedback there letting me know if that is sufficient?

@masci
Copy link
Author

masci commented Nov 22, 2018

Superseded by #194

@masci masci closed this Nov 22, 2018
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.

2 participants