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

simplifies blockstore Column::index implementations #4464

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

behzadnouri
Copy link

@behzadnouri behzadnouri commented Jan 14, 2025

Problem

Column::index implementations are verbose and not very readable.

Summary of Changes

Implemented a macro to simplify the code.

@behzadnouri behzadnouri force-pushed the bs-convert-key-slices branch 2 times, most recently from 573ff81 to d194fb4 Compare January 17, 2025 15:53
@behzadnouri behzadnouri requested a review from steviez January 17, 2025 15:54
@behzadnouri behzadnouri force-pushed the bs-convert-key-slices branch from d194fb4 to ab550f8 Compare January 17, 2025 17:48
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

General changes look good, just a few minor items for consideration on naming / comments

ledger/src/blockstore_db.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_db.rs Outdated Show resolved Hide resolved
@behzadnouri behzadnouri force-pushed the bs-convert-key-slices branch from ab550f8 to 7e2209b Compare January 17, 2025 20:13
@behzadnouri behzadnouri requested a review from steviez January 17, 2025 20:17
@steviez
Copy link

steviez commented Jan 17, 2025

CI was broken in master from the version bump on Rust; Brennan just pushed a fix with #4526 so think you'll need to rebase to current tip to of master to get all greens for CI

The commits adds a small macro to simplify

    pub trait Column {
        fn index(key: &[u8]) -> Self::Index;
        // ...
    }

implementations.
@behzadnouri behzadnouri force-pushed the bs-convert-key-slices branch from 626aea4 to 3daf3d7 Compare January 17, 2025 21:05
@behzadnouri
Copy link
Author

CI was broken in master from the version bump on Rust; Brennan just pushed a fix with #4526 so think you'll need to rebase to current tip to of master to get all greens for CI

done.

Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

LGTM

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