-
Notifications
You must be signed in to change notification settings - Fork 43
feat: write backup files when streaming #254
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
require 'unleash/configuration' | ||
|
||
module Unleash | ||
class BackupFileWriter |
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.
extracted from toggle_fetcher so that we can share it between polling and streaming
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.
Bikeshedding: This might be better served / mode idiomatic by using MixIns in Ruby.
Here is an example:
or
Using include with Modules (Mixins):
The include keyword is used to add a module's methods as instance methods to a class. This is a common way to achieve "mixin" behavior, sharing functionality across unrelated classes.module Greetable def greet_instance puts "Hello from the instance!" end end class MyOtherClass include Greetable # Adds greet_instance as an instance method end my_instance = MyOtherClass.new my_instance.greet_instance # Output: Hello from the instance!
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.
Questions I asked myself:
- do I need to access object state from a mixin method?
- do I want save file operation in fetcher API and streaming processor API?
- is toggle fetcher and streaming event processor a backup file writer (IS-A) or does it use (HAS-A) a backup file writer?
- is this code readable for somone maintaining 5 other SDKs on a daily basis?
It lead me to the current solution.
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.
is this code readable for somone maintaining 5 other SDKs on a daily basis
❤️
I think this solution is fine honestly. If I were going to fuss, it'd be in the direction of making this explicitly stateless rather than the tacit connection that a MixIn brings here. I vote we leave it alone, I think it's a nice improvement to the code structure
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.
Your points make sense! Go for it!
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.
Awesome, thank you!
require 'unleash/configuration' | ||
|
||
module Unleash | ||
class BackupFileWriter |
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.
is this code readable for somone maintaining 5 other SDKs on a daily basis
❤️
I think this solution is fine honestly. If I were going to fuss, it'd be in the direction of making this explicitly stateless rather than the tacit connection that a MixIn brings here. I vote we leave it alone, I think it's a nice improvement to the code structure
About the changes
Changes:
Important files
Discussion points