Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Replace aggregation types with functions #608

Merged
merged 1 commit into from
Mar 21, 2018

Conversation

semistrict
Copy link
Contributor

Fixes: #399

@semistrict semistrict force-pushed the rename-aggr branch 3 times, most recently from ed26a38 to f0b7cea Compare March 20, 2018 18:05
Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

I think only Distribution should be a function, the rest can be constants e.g https://play.golang.org/p/ScwUIbF2Tm4
or inlined

package main

type Aggregation interface {
	aggregate()
}

type countAggregation int

func (countAggregation) aggregate() {}

const CountAggregation = countAggregation(0)

func main() {
	var _ Aggregation = CountAggregation
}

@semistrict
Copy link
Contributor Author

discussed with @rakyll yesterday and she preferred to have all of them be functions for consistency.

@odeke-em
Copy link
Member

Thank you both for the discussion and for the discourse. I see my idea is ugly for consistency as well as for the godoc itself. See
screen shot 2018-03-20 at 3 46 46 pm

Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Thank you @Ramonza and @rakyll for the discourse, I am now convinced that functions are the best option for consistency and a great abstraction to avoid leaking the underlying implementation as displayed by the docs. LGTM @Ramonza.

I've added the labels for api-breaking and do not merge just to keep us aware.

@semistrict
Copy link
Contributor Author

verified that this does not break cloud.google.com/go

@semistrict semistrict merged commit 1f3b709 into census-instrumentation:master Mar 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants