Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add unfinished_candle method #14

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

ivaaaan
Copy link
Contributor

@ivaaaan ivaaaan commented Apr 4, 2024

Another problem I have faced, is that there is no way to get the latest candle from the aggregator.

Input data:

timestamp,price,size
1612137602564000,27334.25,0.00289
1612137664747000,27298.5,0.00909044

TimeRule and AlignedTimeRule will trigger on a first trade, and will return the candle, but there is no way to get the second candle.

I propose to add a new state for candle - finalized, and method close to aggregator. This way, user can easily retrieve the latest candle from the aggregator.

Let me know if there are other ways to achieve this.

@ivaaaan ivaaaan marked this pull request as draft April 5, 2024 03:44
Copy link
Owner

@MathisWellmann MathisWellmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer simply adding a unfinished_candle getter method which provides even more flexibility compared to simply closing an Aggregator.
Its a good feature proposal overall, enhancing the flexibility.
We should be careful to ensure that its clear to a library user that accessing a Candle through a method like this is not recommended as it does not respect the AggregationRule. But for more advanced use cases this is useful

Comment on lines 16 to 21

/// Close current active candle and return it
///
/// # Returns:
/// Last active candle, or None if candle was already finalized
fn close(&mut self) -> Option<Candle>;
Copy link
Owner

@MathisWellmann MathisWellmann Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Close current active candle and return it
///
/// # Returns:
/// Last active candle, or None if candle was already finalized
fn close(&mut self) -> Option<Candle>;
/// Get a reference to an unfinished `Candle`.
/// Accessing a `Candle` using this method does not guarantee that the `AggregationRule` is respected.
/// It is generally advised to call `update` instead and use the resulting `Candle` if its `Some`.
fn unfinished_candle(&self) -> &Candle;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that's what I ended up doing - implemented my own aggregator with method like this. Will update the PR

@ivaaaan ivaaaan marked this pull request as ready for review April 10, 2024 09:41
@ivaaaan
Copy link
Contributor Author

ivaaaan commented Apr 10, 2024

@MathisWellmann updated :)

@ivaaaan ivaaaan changed the title Draft: Finalize candles feat: add unfinished_candle method Apr 10, 2024
@MathisWellmann MathisWellmann merged commit 237f65a into MathisWellmann:main Apr 10, 2024
6 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants