Represent TimeSeries as an interface #14
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 28 28
Lines 553 560 +7
=========================================
+ Hits 553 560 +7 ☔ View full report in Codecov by Sentry. |
|
Any reason @sdcoffey why you havent merged this one? Looks good to me. |
|
Hey guys, sorry for the delay here, for some reason there's an issue with my github settings that causes me to not get notifications about new PRs on this repo. |
There was a problem hiding this comment.
@andviro. Great stuff! I like the spirit behind this PR. I think there's one major blocker to merging this (see below).
Separately, I'd like to understand a little bit better why you felt this change was needed. The lib was originally designed to be flexible in terms of data input. It's true that the TimeSeries as a struct could be implemented using a persistent store, but I'm having a hard time understanding why a little bit of glue code between this lib and your database layer wouldn't do the trick
| @@ -5,21 +5,36 @@ import ( | |||
| ) | |||
|
|
|||
| // TimeSeries represents an array of candles | |||
| type TimeSeries struct { | |||
| Candles []*Candle | |||
| type TimeSeries interface { | |||
There was a problem hiding this comment.
I like the idea behind this, but, because the original TimeSeries struct was exported, I don't think we can just replace the struct def with an interface def, because it will break every project where someone was previously using a concrete TimeSeries struct. I think what we need to do is invert the naming here, s.t. the we give the interface another name (maybe ITimeSeries to borrow from C++, but I'm sure you can suggest something better) and keep TimeSeries bound to the struct def. WDYT?
There was a problem hiding this comment.
Yes, there must be a more elegant way. Sorry for bothering you too early. I need to think a bit more about it all.
| LastCandle() *Candle | ||
| GetCandle(int) *Candle | ||
| GetCandleData() []*Candle | ||
| AddCandle(*Candle) bool |
There was a problem hiding this comment.
in terms of the methods in this interface, I want to make sure we're being deliberate about what we choose, because once we commit to these we can't remove or change any of them. Based on the usages in this repo, I think we could get away with something like this:
type ITimeSeries interface {
AddCandle(*Candle) bool
Candles() []*Candle
}the others seem like convenience methods to access data at a particular location.
There was a problem hiding this comment.
Yeah, I understand your concerns. I guess that was a bit premature decision, disregarding legacy code. And presenting as small interface as possible is also a valid point. As for motivation behind this PR, it's just what I think is usually right: parts that "do" or "provide" stuff are usually interfaces, and parts that circulate around as DTOs are usually structs. The time series looks like a "provider" of Candles to me, hence it's an interface. But you're right, I should have thought it all through more carefully. For instance, some indicators use internal caches that also are slices. If we decide to go the whole way, cache must be more abstract too. For example, "infinite" time series implemented as some kind of ring buffer will not use too much memory, but cache as a slice definitely will.
There was a problem hiding this comment.
No worries! feel free to iterate on this and tag me for another review. Thanks for your contribution! It's exciting to see folks wanting to work on this and make it better
Hi there!
I've converted TimeSeries from struct to interface, more alike to ta4j. I think it's beneficial because TS can be implemented differently, using Influxdb for example.