Skip to content

Some refactorings#1

Merged
HaveFunTrading merged 6 commits intoHaveFunTrading:mainfrom
dkumsh:dq
Jan 26, 2026
Merged

Some refactorings#1
HaveFunTrading merged 6 commits intoHaveFunTrading:mainfrom
dkumsh:dq

Conversation

@dkumsh
Copy link
Contributor

@dkumsh dkumsh commented Jan 23, 2026

Hi Tom, we are using your crate and I proposes this set of changes, some micro optimizations and code cleanup. Please have a look. I hope you'll like it.

@HaveFunTrading
Copy link
Owner

hi Dmitry! thank you for raising the PR. I had a quick look and the proposed changes make a lot of sense. I will aim to review in detail over this weekend.

@HaveFunTrading
Copy link
Owner

@dkumsh the pipeline failed (looks mostly because of clippy/code format), I always run these commands before commit

cargo clippy --no-deps --all-targets --all-features  -- -D warnings
cargo fmt

The msrv you would need to bump in Cargo.toml to next min supported version

@dkumsh dkumsh force-pushed the dq branch 2 times, most recently from d62f132 to 4fee1fe Compare January 24, 2026 23:01
@dkumsh
Copy link
Contributor Author

dkumsh commented Jan 24, 2026

Hi Tom,

I fixed the PR by making MetricsHandle explicitly Send/Sync (with a safety note that backends must be thread‑safe or used strictly single‑threaded). This resolves the LazyLock Sync error from the allocator. I also moved the [release] table to [workspace.metadata.release] to silence the Cargo warning and removed an extra blank line in the Histogram docs that Clippy flagged.

@HaveFunTrading
Copy link
Owner

@dkumsh I have finished the review, added few comments; otherwise looks very good

@dkumsh
Copy link
Contributor Author

dkumsh commented Jan 25, 2026

@HaveFunTrading Thanks for the review. I don't see the inline comments yet - could you please resubmit the review so I can address them.

@HaveFunTrading HaveFunTrading merged commit cd41767 into HaveFunTrading:main Jan 26, 2026
4 checks passed
@HaveFunTrading
Copy link
Owner

thanks for addressing @dkumsh , I have merged and released new version 0.0.16. Let me know if you notice any issue.

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.

2 participants