8000 Move `base58` to the crate root by tcharding · Pull Request #1417 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Move base58 to the crate root #1417

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

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

tcharding
Copy link
Member

Move base58 module to the carte root, direct rustfmt to not format the digits array. Run formatter as a separate patch.

@tcharding tcharding added P-high High priority trivial Obvious, easy and quick to review (few lines or doc-only...) aggregate release notes mention labels Nov 28, 2022
@apoelstra
Copy link
Member

This one, like #1400, should eventually go in bitcoin-str, so we should decide on a name and create a module.

@apoelstra
Copy link
Member

@Kixunil let's move discussion of bitcoin-str here from 1400 so that we're not interleaving project discussion with workflow discussion and Connor's actual PR.

How do you feel about creating a module called string, which we'll later (after next release say) promote to a crate bitcoin-string. We'll put base58 in there as well as Connor's new trait, and other things as we see fit. We'll also re-export bitcoin-bech32 from there.

@Kixunil
Copy link
Collaborator
Kixunil commented Nov 29, 2022

For base58 I'd prefer a separate crate (at least long-term).

string module sounds good as a facade but I'm not convinced it needs to be a crate.

@apoelstra
Copy link
Member

Ok, if we want base58 to be its own crate then we can move it to the root here. concept ACK (though needs rebase)

@tcharding
Copy link
Member Author

Rebase only, no other changes.

apoelstra
apoelstra previously approved these changes Nov 30, 2022
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK c12a17b6a754536569efcb3d090a072a14b52a01

Kixunil
Kixunil previously approved these changes Nov 30, 2022
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK c12a17b6a754536569efcb3d090a072a14b52a01

@apoelstra
Copy link
Member
apoelstra commented Nov 30, 2022

Needs rebase. (Tried localy, it's a real merge failure). Caused by #1373

In preparation for removing the `util` module move the `base58` module
to the crate root. This is likely not the final resting place for this
module but it is a step in the right direction.

Includes addition of rustfmt attribute to skip formatting the digits
array. No other changes to the `base58` module.
Run `cargo +nightly fmt` and commit the changes to the `base58` module.
No manual changes, only those done by the formatter.
@tcharding tcharding dismissed stale reviews from Kixunil and apoelstra via 4c8570b November 30, 2022 22:56
@tcharding
Copy link
Member Author

Rebase only, no other changes.

Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 4c8570b

Copy link
Member
@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 4c8570b

@sanket1729
Copy link
Member

Merging this as it marked as high prio

@sanket1729 sanket1729 merged commit 22c6406 into rust-bitcoin:master Dec 1, 2022
@tcharding tcharding deleted the 11-29-move-base58 branch December 2, 2022 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aggregate release notes mention P-high High priority trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0