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

Improve type inference in SimpleCollector.Builder #86

Merged

Conversation

andersschuller
Copy link
Contributor

I have tweaked some of the generics on SimpleCollector and SimpleCollector.Builder in order to improve the type inference and compile-time guarantees when using builders. This should fix #65, but it actually also fixes an issue for Java users. Currently, the following code compiles, but fails at runtime with a ClassCastException:

Gauge gauge = Counter.build().name("name").help("help").register();

This is because the return types of register and create are currently inferred at the call site, rather than being determined by the builder itself. I have therefore added a second type parameter to the builder, which is the type of collector it builds.

I have also removed the second type parameter from SimpleCollector, since it was unused. I think the intention of this parameter was to use it as I am using the new builder type parameter, but since the builder classes are static this was not possible.

This change breaks the API for anyone extending SimpleCollector, but since the documentation says "you should never need to subclass this class", I'm assuming this is not really an issue.

@brian-brazil
Copy link
Contributor

Thanks for this it looks good to me, @willfleury would you mind taking a look at this as you're more an expert here than I?

Are there any tests we could add to prevent an accidental regression here in the future?

This change breaks the API for anyone extending SimpleCollector, but since the documentation says "you should never need to subclass this class", I'm assuming this is not really an issue.

Indeed.

@willfleury
Copy link
Contributor

Looks good. Much better than we had it originally

@brian-brazil
Copy link
Contributor

Great, thanks!

brian-brazil added a commit that referenced this pull request Oct 11, 2015
…ence

Improve type inference in SimpleCollector.Builder
@brian-brazil brian-brazil merged commit 6c73020 into prometheus:master Oct 11, 2015
@andersschuller andersschuller deleted the improve-builder-type-inference branch October 11, 2015 10:47
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.

SimpleCollector.Builder register's type inference goes wrong way
3 participants