Add enumeration expectations #9447
Conversation
TylerHelmuth
added
the
Skip Changelog
CONTRIBUTING.md
Outdated
| - Enumerations should use either `int` or `string` as the underlying type | ||
| - Enumeration name should be: | ||
| - Prefixed with a clarifying name *unless* the package name describes the entity of the enumeration. | ||
| - Suffixed with `Type` |
There was a problem hiding this comment.
Starting with a strict statement here so we can poke holes at this.
Immediately an enumeration like component.Kind, which has constants like ReceiverKind, would not match this role.
There was a problem hiding this comment.
Why is Type as a literal required? An enum const should be prefixed with its type. E.g. configtelemetry.LevelNone when we have type Level int32
There was a problem hiding this comment.
An enum const should be prefixed with its type.
Agreed, I'll update that rule.
I think the largest question of the PR is "When should an Enum use Type in its name or be named Type". I've started the PR by claiming "All Enums should include Type in the name".
I don't think that is correct, but I am struggling to put into words the situations when Type is appropriate.
There was a problem hiding this comment.
I don't think that is correct, but I am struggling to put into words the situations when Type is appropriate.
I don't think we should dictate using Type as literal. Sometimes it may be a good fit, sometimes not
There was a problem hiding this comment.
@dmitryax Updated to try and put into words what Type represents, which I am currently interpreting as a sense of limited categorization.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9447 +/- ##
==========================================
+ Coverage 90.24% 90.25% +0.01%
==========================================
Files 344 344
Lines 17932 17932
==========================================
+ Hits 16182 16185 +3
+ Misses 1421 1419 -2
+ Partials 329 328 -1 ☔ View full report in Codecov by Sentry. |
CONTRIBUTING.md
Outdated
| - Enumerations should use either `int` or `string` as the underlying type | ||
| - Enumeration name should be: | ||
| - Prefixed with a clarifying name *unless* the package name describes the entity of the enumeration. | ||
| - Suffixed with `Type` |
There was a problem hiding this comment.
Why is Type as a literal required? An enum const should be prefixed with its type. E.g. configtelemetry.LevelNone when we have type Level int32
There was a problem hiding this comment.
To be clear: in the case of configcompression this would mean basically doing the changes in #9417, right? @dmitryax do you agree?
edit: No, for what it would entail see #9447 (comment)
I disagree, because I think having "compression" as suffix in the package name + https://github.com/open-telemetry/opentelemetry-collector/pull/9447/files#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055R261 should lead us to rename the enum as |
|
Ah, you are right @bogdandrutu 👍 |
Description:
Adds expectations for how the repository handle enumerations
Link to tracking Issue:
Relates to #9388