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 support for units and includeZero in Yaxis #187

Merged
merged 1 commit into from
Nov 12, 2018
Merged

Add support for units and includeZero in Yaxis #187

merged 1 commit into from
Nov 12, 2018

Conversation

nyanshak
Copy link
Collaborator

  • Adds IncludeZero to support whether to always include zero or not.
    Default is to always include zero.
  • Adds IncludeUnits (units in Datadog json) to determine whether to show
    the unit label to the left of the yaxis or not.

* Adds IncludeZero to support whether to always include zero or not.
Default is to always include zero.
* Adds IncludeUnits (units in Datadog json) to determine whether to show
the unit label to the left of the yaxis or not.
@nyanshak
Copy link
Collaborator Author

Questions for other reviewers:

  • I've made Datadog's "units" field into IncludeUnits to hopefully make the intent more clear. Should I change it to Units to more closely match?
  • I tested IncludeZero and IncludeUnits with string values as well "true" and "false". Datadog's API accepts and returns them, but the UI does NOT handle them correctly. I believe the most correct thing to do here is keep the type as *bool and fail if a string is received / not support string types. Does that make sense?

@gphat
Copy link

gphat commented Oct 27, 2018

I came here to make a PR like this only to see that @nyanshak had already done it. I'm not involved in this project but would love to use this PR and am 👍 to the PR itself. :)

@nyanshak nyanshak requested a review from ojongerius October 27, 2018 14:58
@ghost
Copy link

ghost commented Oct 29, 2018

LGTM. The failing tests look like failures for some Go versions due to golint. Probably worth checking if Travis CI needs some other args for those versions in another PR.

I've made Datadog's "units" field into IncludeUnits to hopefully make the intent more clear. Should I change it to Units to more closely match?

I think IncludeUnits is clearer. The struct already differs exactly from the UI (due to the the Auto stuff), so it doesn't bother me.

I tested IncludeZero and IncludeUnits with string values as well "true" and "false". Datadog's API accepts and returns them, but the UI does NOT handle them correctly. I believe the most correct thing to do here is keep the type as *bool and fail if a string is received / not support string types. Does that make sense?

The type is fine, IMO, unless consumers can get into an ugly state that's hard to debug (like the auto issue). I'd rather the UI behave poorly than have something downstream like Terraform breaks. Also, did you file a bug upstream about it?

@nyanshak
Copy link
Collaborator Author

Consumers could get into a weird state if someone else were to edit this incorrectly in the json editor or the API. Unlike the other times we've seen similar problems (like "5" vs 5 in max/min), using string types here does not actually work in the UI.

I just raised this through Datadog support, so... fingers crossed :) Thanks for reviewing, @yfronto

@gphat
Copy link

gphat commented Nov 10, 2018

Hey all! Anything I can to to help move this along? Thanks!

@nyanshak nyanshak merged commit f3f6d2f into zorkian:master Nov 12, 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