Skip to content

Various improvements - #39

Closed
ayrat555 wants to merge 1 commit into
rust-bitcoin:masterfrom
ayrat-forks:master
Closed

Various improvements#39
ayrat555 wants to merge 1 commit into
rust-bitcoin:masterfrom
ayrat-forks:master

Conversation

@ayrat555

@ayrat555 ayrat555 commented Feb 19, 2023

Copy link
Copy Markdown
Contributor
  • fix clippy warnings

@ayrat555

ayrat555 commented Feb 19, 2023

Copy link
Copy Markdown
Contributor Author

@apoelstra @stevenroose hey. can you please review this pr?

the main change in this pr is all_words function. currently there is no way to get a list of words

@tcharding

Copy link
Copy Markdown
Member

@apoelstra isn't involved in this crate, Steven is out of action right now, I can take a look for you though :)

@tcharding

Copy link
Copy Markdown
Member

Can you put the clippy fixes in a separate commit please and the "add words" stuff in a commit on its own. Will make it much easier to review since I'm not super familiar with this code. FYI don't hold your breath for this to get merged any time soon, movement on this crate is a bit slow at the moment I'm afraid. We are doing our best :)

@ayrat555

Copy link
Copy Markdown
Contributor Author

@tcharding done!

@tcharding tcharding left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Woops, somehow when I did the original review these two comments didn't get posted. The rest looks good to me.

Comment thread .travis.yml Outdated
- rust: nightly
env: BENCHES=true
- rust: 1.29.0
- rust: 1.67.1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the MSRV, can't change this version. You made me realize that I missed updating it in #26

Comment thread Cargo.toml Outdated
[lib]
name = "bip39"
path = "src/lib.rs"
edition = "2021"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't work with the current (or upcoming MSRV) I'm afraid.

@tcharding

tcharding commented Feb 20, 2023

Copy link
Copy Markdown
Member

If you remove the edition 2021 change and the toolchain version then it should be good to go!

@tcharding

Copy link
Copy Markdown
Member

Can you squash the last commit please? That way there are no changes later in the series of patches that change something in an earlier patch. Makes review easier for the next guy. Thanks

@tcharding

Copy link
Copy Markdown
Member

You'll need to amend the PR description too :)

@ayrat555

Copy link
Copy Markdown
Contributor Author

@tcharding addressed your comments.

if Steven is out of action, maybe he can give the ownership to someone else?

@tcharding

Copy link
Copy Markdown
Member

Nah, we'll wait for him. Its open source, you can always fork it if you want to go a different direction.

@stevenroose

Copy link
Copy Markdown
Collaborator

The versioning part of this is handled in another MR and the all_words function seems to be identical to .language().word_list(), when word_list will be exposed in v1.1.0, see #40.

@ayrat555

Copy link
Copy Markdown
Contributor Author

@stevenroose I removed this funciton

@tcharding

Copy link
Copy Markdown
Member

This PR is only clippy changes now, perhaps #27 is better for that. @ayrat555 can you close if you are ok with waiting for #27 to go in?

@stevenroose

Copy link
Copy Markdown
Collaborator

Everything in this MR merged in some form or another except #27 indeed. Closing.

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.

3 participants