-
Notifications
You must be signed in to change notification settings - Fork 78
Make public enums into structs or hide them (part 1) #1025
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
Conversation
|
|
||
| public static func leadershipChange(_ leadershipChange: LeadershipChange) -> Event { | ||
| .init(.leadershipChange(leadershipChange)) | ||
| } |
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.
Hmmm... so this one I'd kind of want to keep as an enum to be honest :\
The primary way to use it is to switch over it in an async for loop of events...
Perhaps we should do this case _PLEASE_DO_NOT_EXHAUSTIVELY_MATCH_THIS_ENUM_NEW_CASES_MIGHT_BE_ADDED_IN_THE_FUTURE trick for this one specifically... other SSWG libs do this for such cases.
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.
amended 7ea7b1f
Internally we just log error and/or ignore the "fake" event. Is that ok? I don't think we should do default because we will forget to update the switch when we add new event in the future. It doesn't feel right to throw error or crash either.
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.
Agreed, that looks good 👍
|
|
||
| extension OpLog.SequencedOp.SequenceRange: Codable { | ||
| public enum DiscriminatorKeys: String, Codable { | ||
| enum DiscriminatorKeys: String, Codable { |
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.
yep good to keep internal!
| /// Upon noticing that this member is marked as [.down], initiate a shutdown. | ||
| public static func gracefulShutdown(delay: Duration) -> OnDownActionStrategySettings { | ||
| .init(.gracefulShutdown(delay: delay)) | ||
| } |
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.
good one, thank you -- yes this is a good candidate for this treatment, LGTM
|
Downing and OpLog LGTM, the Event I guess we may need to do the terrible approach instead... :-/ |
|
Flaky #971 |
Part of #999