[configcompression] Rename CompressionType to Type
#9416
Conversation
TylerHelmuth
force-pushed
the
configcompression-rename-enum
branch
from
2107ef2
to
ace7fdd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9416 +/- ##
==========================================
- Coverage 90.44% 90.42% -0.02%
==========================================
Files 346 346
Lines 18106 18106
==========================================
- Hits 16376 16373 -3
- Misses 1399 1401 +2
- Partials 331 332 +1 ☔ View full report in Codecov by Sentry. |
| TypeDeflate Type = "deflate" | ||
| TypeSnappy Type = "snappy" | ||
| TypeZstd Type = "zstd" | ||
| typeNone Type = "none" |
There was a problem hiding this comment.
Should "none" be a valid type? By valid I mean public to be used.
There was a problem hiding this comment.
Based on the original PR's conversation there wasn't a public use case for none: #4651 (comment). Until the last 2 weeks there haven't been any material changes to the package so I'm guessing that there still isn't any public use case.
There was a problem hiding this comment.
This is something we could do after 1.0, I think we can keep it as is and expose in the future if we have a specific need
| none, | ||
| empty: | ||
| func (ct *Type) UnmarshalText(in []byte) error { | ||
| switch typ := Type(in); typ { |
There was a problem hiding this comment.
nit: Sorry, but my mind goes crazy when I see a switch used like an if :)) Can we fix this probably in a separate PR :)
There was a problem hiding this comment.
Are you objecting to typ := Type(in); typ or the switch statement in general?
There was a problem hiding this comment.
In general I don't like switch statements with only 2 branches, the are if's no? :)) But is a nit and not a blocker for sure.
TylerHelmuth
force-pushed
the
configcompression-rename-enum
branch
from
3fd14b8
to
4bd090e
Compare
| TypeDeflate Type = "deflate" | ||
| TypeSnappy Type = "snappy" | ||
| TypeZstd Type = "zstd" | ||
| typeNone Type = "none" |
There was a problem hiding this comment.
This is something we could do after 1.0, I think we can keep it as is and expose in the future if we have a specific need
|
I think we can merge this on Monday EU morning to get it to next week's release, unless there are objections by then (all pending conversations don't affect public API and can be solved on separate PRs) |
| none, | ||
| empty: | ||
| func (ct *Type) UnmarshalText(in []byte) error { | ||
| switch typ := Type(in); typ { |
There was a problem hiding this comment.
In general I don't like switch statements with only 2 branches, the are if's no? :)) But is a nit and not a blocker for sure.
|
Please rebase |
Description:
If we choose to go with the rename.
Link to tracking Issue:
Related to #9388