-
Notifications
You must be signed in to change notification settings - Fork 34
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
use a sealed class for Feed to support a dynamic 'other' option #119
Conversation
Hey @mmoghaddam385, We might be able to add a custom option to the For example, add a custom feed option in the enum class Feed(val url: String) {
// ...
CustomFeed("customfeed"), // Define custom feed
} Then, add a custom constructor in the class PolygonWebSocketClient(
// ... existing parameters
private val customFeedURL: String? = null // Add a custom feed URL parameter
) {
init {
// If customFeedURL is provided, update the URL of CustomFeed
customFeedURL?.let {
Feed.CustomFeed.url = it
}
}
// ... rest of the code
} On the cliet side, we can then create an instance with a custom feed URL like this: val client = PolygonWebSocketClient(
apiKey = "XXXX",
feed = Feed.CustomFeed, // Use the custom feed
customFeedURL = "localhost" // Set the custom feed URL to localhost
// ...
) So, for example, say that you wanted to connect to a custom URL, maybe "localhost", you could do that by setting the custom feed option. This seems to work into the existing model but if we come out with something totally new then they are going to need to message parser and model updates too. So, this only works in really limited based on the Feed side where you might want to test an alternative host. |
Hmm I think there are a couple problems with a custom enum approach. Since the enum is global you wouldn't be able to have two custom feed connections running in the same app. Maybe not a common situation but if someone tried to do it they would see some confusing behavior. I also think having a second param that overrides an existing one can be a confusing UX, particularly if we end up with two of them (one for Market another for Feed). If we use sealed classes like in this PR I don't think we need to change anything in the message parsers, the same |
Makes sense to me. Fixed in #122. |
Closing this in favour of #122. |
Experimenting with how it would look to use a sealed class instead of an enum for
Feed
. This would let us support a dynamic "other" attribute to make the SDK more flexible for when we add new feed types. I see this benefit as two-fold:If we like this pattern, we should do the same for the
Market
enum.The only downside to this is that the Java interaction becomes a bit more awkward, since Java usages must access the "enum" through the
.INSTANCE
member as shown in the updated Java sample. LMK what you think about this tradeoff.