Refactor metrics/cloudwatch #1202
Conversation
- Export `option` type for better documentation - Add documentation for exported option functions - Use `CloudWatch.apply` method and remove unused `Percentiles` type
metrics/cloudwatch/cloudwatch.go
Outdated
| for _, opt := range options { | ||
| cw.apply(opt) | ||
| } |
There was a problem hiding this comment.
Can we just remove the apply method altogether?
There was a problem hiding this comment.
It's actually more safer than a simple function. apply cannot be called outside of the package, but with the current Option type it's possible to modify the state of Cloudwatch after the initialization.
I'd instead change the signature of Option to this:
type Option interface{
apply(*CloudWatch)
}There was a problem hiding this comment.
I'd instead change the signature of Option to this:
Strong -1. Option interfaces are an antipattern, and especially those that take this form, i.e. an exported interface with an unexported method. type Option func(*Cloudwatch) keeps the Options grouped with the type in the docs, which is important.
with the current Option type it's possible to modify the state of Cloudwatch after the initialization.
I understand your point, but I don't think this is a meaningful risk, as only Options defined in package cloudwatch have access to the unexported fields of the struct.
There was a problem hiding this comment.
I think we can remove the apply method, because cloudwatch.New(namespace, svc, nil) isn't norm.
optiontype for better documentationUseCloudWatch.applymethod and remove unusedPercentilestypeCloudWatch.applymethod andPercentilestype