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

This library is not solely dependent on Rust #1904

Merged
merged 1 commit into from Jun 12, 2023

Conversation

roy9495
Copy link
Contributor

@roy9495 roy9495 commented Jun 8, 2023

Fix: #1867

Docs claim this library is pure rust but it depends on libsecp256k1 (and optionally libbitcoinconsensus)
So, I changed the lib.rs file accordingly.

@apoelstra
Copy link
Member

@roy9495 could you please update your local master to the current one on Github, and rebase this PR?

//! It is also written entirely in Rust to illustrate the benefits of strong type
//! safety, including ownership and lifetime, for financial and/or cryptographic
//! software.
//! Except for its dependency on libsecp256k1(and optionally libbitcoinconsensus),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//! Except for its dependency on libsecp256k1(and optionally libbitcoinconsensus),
//! Except for its dependency on libsecp256k1 (and optionally libbitcoinconsensus),

@roy9495
Copy link
Contributor Author

roy9495 commented Jun 8, 2023

@roy9495 could you please update your local master to the current one on Github, and rebase this PR?

Sure sir

@tcharding
Copy link
Member

When you implement some fix from review can you make the change to the original commit. This way a PR does not contain commits that fix things in previous commits which makes review hard for the next reviewer. Thanks

@roy9495
Copy link
Contributor Author

roy9495 commented Jun 8, 2023

When you implement some fix from review can you make the change to the original commit. This way a PR does not contain commits that fix things in previous commits which makes review hard for the next reviewer. Thanks

Sir, can I squash the commits ?

@tcharding
Copy link
Member

yes

@apoelstra
Copy link
Member

@roy9495 yes, in this case you should just squash them, because the resulting single commit will be small and self-contained.

In general, you want to produce PRs that are a series of well-defined, well-motivated, self-contained commits which each compile and pass tests independently. For small changes, "squash everything into one commit" is a perfectly reasonable way to do this. For bigger ones this can take some practice and repeated invocation of git rebase -i, or even doing a git reset --soft master to go back to master with all your changes uncommitted and ready to be committed piece-meal. Or even just opening a new worktree (or fresh clone of the repo :P) and copy/pasting your changes in, committing as you go.

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 654f58d

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 654f58d

@apoelstra apoelstra merged commit 089b4e2 into rust-bitcoin:master Jun 12, 2023
29 checks passed
@roy9495
Copy link
Contributor Author

roy9495 commented Jun 12, 2023

Thank you

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.

The docs claim this library is pure rust but it depends on libsecp256k1 (and optionally libbitcoinconsensus)
4 participants