Allow Summaries with empty objectives and deprecate DefObjectives #262
Conversation
| @@ -54,6 +54,9 @@ type Summary interface { | |||
| } | |||
|
|
|||
| // DefObjectives are the default Summary quantile values. | |||
| // | |||
| // Deprecated: DefObjectives will be removed in v0.10. The default will be a | |||
There was a problem hiding this comment.
Hm, it would be handy to still provide a package variable given how common these quantiles are. I doubt many people have a good feeling what error value to choose and would copy and paste it anyway.
There was a problem hiding this comment.
Handy to provide these values in v0.10, even if they aren't used internally to the package?
There was a problem hiding this comment.
I agree. While users shouldn't really be using these in the first place, it's a breaking change and this will help quell some of the rumblings it'll cause.
There was a problem hiding this comment.
Sure, we can leave them in place, but I'd still rename then. Let's decide about that later (when we actually change the default behavior). For now, nothing changes. I have changed the comment to not suggest that they will be removed (rather: not used as default anymore).
|
This PR is fine to me, but as @grobie is the guru who has his finger on the pulse of what the users want, I defer to him |
This also updates all tests and examples to use explicitly set objectives. In v0.10, DefObjectives will be completely removed, and the default Summary will have no objectives then. Fixes #118
beorn7
force-pushed
the
beorn7/summary
branch
from
ffe6554 to
9c4b778
Compare
|
As per personal discussion, I have removed the funny comments from the examples. They were grammatically wrong, and Matt taught me to not make examples funny (but relevant). Ironically, those examples were of his own (at least they were not mine, I found them when I entered the project 3y ago...). |
|
@stuartnelson3 Do you think comments are addressed? |
|
looks good |
This also updates all tests and examples to use explicitly set
objectives.
In v0.10, DefObjectives will be completely removed, and the default
Summary will have no objectives then.
Fixes #118
@stuartnelson3