Add support for gauges. #10
Conversation
|
Nothing in here looks heinous to me, but I don't know much about tallier. Side note, in the future it might be a good idea to separate gofmt changes for existing code into a separate commit (and even have precommit hooks that check formatting). 🐟 |
Very 👍 |
| @@ -28,6 +29,7 @@ type Sample struct { | |||
| valueType SampleType | |||
| sampleRate float64 | |||
| stringValue string | |||
| replace bool | |||
There was a problem hiding this comment.
This guy kinda bums me out. Is there a way to make the booleans more obvious elsewhere? It's really hard for me to know when seeing a structure instantiated what false being the last parameter means.
There was a problem hiding this comment.
As discussed in person, perhaps just having true/false variants of the gauge functions rather than taking a boolean would be a more readable approach.
There was a problem hiding this comment.
On further thought, I think we could do this by having a value type of GAUGE_REPLACE and GAUGE_MODIFY which could then trigger different codepaths. This allows us to dodge adding another field onto the Sample struct which is the cause for so many noisy updates in the tests etc.
|
💅 question about boolean readability |
| replace = false | ||
| value, _ = ParseFloat(sample) | ||
|
|
||
| if _, err := strconv.Atoi(string(sample[0])); err == nil { |
There was a problem hiding this comment.
Looking at this further, this is icking me out a bit: is this trying to figure out if the first character is a + or - by seeing if that character by itself can be turned into an integer?
|
re-:nail_care: just for the heck of it. Was looking at merging this patch and found some issues while digging in. Whatcha think? |
I apologize in advance for bad Go
👓 @spladug @dforsyth