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

Remove genesis_block lazy initialization #756

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

shobitb
Copy link
Contributor

@shobitb shobitb commented Sep 18, 2022

Description

This commit fixes #752

cargo test runs successfully.

Notes to the reviewers

Hi, newbie here learning Rust and BDK! I've removed the lazy_static block in this commit, and when learning about lazy_static also came across something called once_cell, soon to be available in stdlib. It's like lazy_static but faster and is not a macro. Shall I keep the lazy_static and create an issue to switch to once_cell in the future, or remove the lazy_static (as I've done) and create an issue anyway?

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@danielabrozzoni
Copy link
Member

Hey, thanks for your contribution :)
There are some compilation errors when compiling with the compact_filter feature, you can see them if you compile like this:

cargo build --features compact_filters

Regarding lazy_static vs once_cell: in this specific case I'd say it's better to use genesis_block (as you're doing), but in the other cases, sure, once_cell would be better! Feel free to open an issue :)

@shobitb
Copy link
Contributor Author

shobitb commented Sep 22, 2022

Thanks @danielabrozzoni! I've updated my commit and ensured compact_filters compiles successfully.

bdk % cargo build --features compact_filters
   Compiling bdk v0.22.0 (/Users/shobit/bdk)
    Finished dev [unoptimized + debuginfo] target(s) in 27.82s

Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

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

You should also update the Cargo.toml to remove lazy_static from the compact_filters feature since it's not needed anymore.

Actually I think you may even be able to remove the lazy_static dependency (not the dev-dependency which is used in the tests) entirely, I don't think it's used anywhere else

@shobitb
Copy link
Contributor Author

shobitb commented Sep 24, 2022

You should also update the Cargo.toml to remove lazy_static from the compact_filters feature since it's not needed anymore.

Actually I think you may even be able to remove the lazy_static dependency (not the dev-dependency which is used in the tests) entirely, I don't think it's used anywhere else

Got it, and thank you for taking a look! I've updated the commit after removing the dependency (but not dev-dependency).

Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

Tested ACK. Thanks for the contribution!

@danielabrozzoni
Copy link
Member

Code looks good to me, can you please squash all commits? You have three separate commits with the same title/message, it should be just one.

https://stackoverflow.com/a/5189600

This commit contains a change to address issue bitcoindevkit#752

cargo test runs successfully.
@shobitb
Copy link
Contributor Author

shobitb commented Sep 26, 2022

Code looks good to me, can you please squash all commits? You have three separate commits with the same title/message, it should be just one.

https://stackoverflow.com/a/5189600

Done! Thanks for the link.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK e6f2d02

@notmandatory notmandatory merged commit b14e4ee into bitcoindevkit:master Sep 26, 2022
@afilini afilini mentioned this pull request Oct 7, 2022
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remove genesis_block lazy initialization
5 participants