Skip to content
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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants