-
Notifications
You must be signed in to change notification settings - Fork 553
networking: add constants for statuses #3329
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
networking: add constants for statuses #3329
Conversation
I'm leaning towards just providing the constants without changing the types, because that's going to be a pain for users as shown with the failing CI unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion is that these constants can be of type string without additional typing. If designed from scratch they could be numeric Go enums, but as a retrofit I don't see a lot of value balancing the lack of backwards compatibility.
PortStatusActive PortStatus = "ACTIVE" | ||
PortStatusBuild PortStatus = "BUILD" | ||
PortStatusDown PortStatus = "DOWN" | ||
PortStatusError PortStatus = "ERROR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the package name "ports", the identifiers for these constants can be just "StatusActive" without fear of confusion.
This avoids the need to define these constants in consumer code. Router also has a status field, but there's no documentation for it, so let's not guess what the valid statuses are.
ced40b2
to
eda8452
Compare
This avoids the need to define these constants in consumer code.
Router also has a status field, but there's no documentation for it, so
let's not guess what the valid statuses are.