-
Notifications
You must be signed in to change notification settings - Fork 594
Display error message for NetworkErrorCode #1678
Conversation
@@ -30,4 +33,12 @@ enum NetworkErrorCode { | |||
TIMEOUT | |||
}; | |||
|
|||
static inline std::ostream& operator<<(std::ostream& os, const NetworkErrorCode code) { | |||
std::string strs[] = { "OK", "Connection error", "Read error", "Write 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.
Can these instead be bound directly to the enum values themselves, instead of managed separately?
http://en.cppreference.com/w/cpp/language/enum
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.
I am not aware of any C++ syntax for this, we may achieve it by a wrapper class or something like that, but I doubt it is worth the effort to improve it. Does it hurt readability to you?
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.
this looks useful https://aantron.github.io/better-enums/tutorial/Conversions.html#Strings and easy to include in, if we bother to use it at all...
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.
There is always some way to do it. Again, why do you think it is worth our effort to do it? For me, we only have one case so far, no need to bother a complex way to make it unnecessarily elegant. If you believe it is worth, you are welcome to do it in a separate PR.
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.
just pointing out. i have no opinion since C++ language does not have the capability of allowing people to use enum reflection easily.
can you check out the CI? |
@objmagic IntegrationTest_MultiSpoutsMultiTasks fails. |
There is a new CI failure:
|
Thanks. I'll just restart the CI. |
The PR CI is fine. Feel free to merge it. |
The PR CI check passed. Merged. |
This is to resolve #1654.