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.

Sign up for GitHub

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

Jump to bottom

Fix clippy warnings for new stable rust 1.50.0 #282

Merged
merged 6 commits into from
Feb 13, 2021
Merged

Fix clippy warnings for new stable rust 1.50.0 #282

merged 6 commits into from
Feb 13, 2021

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Feb 11, 2021 •
edited

Description

Two new clippy warning are showing up with the new stable version of rust 1.50.0.

The first clippy warning unnecessary_wraps only required removing the unnecessary Result wrapper for the policy::finalize() and compact_filters peer::_recv() functions.

Slightly more changes were required for the second warning wrong_self_convention which requires renaming the ToWalletDescriptor::to_wallet_descriptor() function to into_wallet_descriptor() to satisfy the naming convention that into_ methods take self and to_ methods take &self.

Notes to the reviewers

Once this is merged to master I'll cherry pick it to the `release/0.4.0' branch.

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory notmandatory changed the title Fix/clippy 150 Fix clippy warnings for new stable rust 1.50.0 Feb 11, 2021
@notmandatory notmandatory force-pushed the fix/clippy_150 branch 2 times, most recently from d2f22cd to 3ebf720 Compare February 11, 2021 23:24
@notmandatory notmandatory marked this pull request as ready for review February 11, 2021 23:40
@notmandatory notmandatory mentioned this pull request Feb 12, 2021
Merged
8 tasks
tcharding
tcharding reviewed Feb 13, 2021
View reviewed changes
Copy link
Contributor

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Typo in brief message of commit: 25222b4 Fix clippy warning 'unnecissary_wraps' (unnecessary misspelled).

Thanks

src/descriptor/mod.rs Outdated
@@ -74,15 +74,15 @@ pub type HDKeyPaths = BTreeMap<PublicKey, KeySource>;
/// Trait for types which can be converted into an [`ExtendedDescriptor`] and a [`KeyMap`] usable by a wallet in a specific [`Network`]
pub trait ToWalletDescriptor {
Copy link
Contributor

@tcharding tcharding Feb 13, 2021 •
edited

Choose a reason for hiding this comment

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

Perhaps this should be changed as well to IntoWalletDescriptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes makes sense. Fixed.

CHANGELOG.md Outdated
@@ -27,6 +27,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Wallet
#### Changed
- Removed the explicit `id` argument from `Wallet::add_signer()` since that's now part of `Signer` itself
- Renamed `ToWalletDescriptor::to_wallet_descriptor` to `ToWalletDescriptor::into_wallet_descriptor`

### Policy
Copy link
Contributor

Choose a reason for hiding this comment

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

This line has trailing whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 fixed.

src/descriptor/template.rs Outdated
@@ -70,12 +70,12 @@ pub trait DescriptorTemplate {
/// Turns a [`DescriptorTemplate`] into a valid wallet descriptor by calling its
/// [`build`](DescriptorTemplate::build) method
impl<T: DescriptorTemplate> ToWalletDescriptor for T {
Copy link
Contributor

@tcharding tcharding Feb 13, 2021 •
edited

Choose a reason for hiding this comment

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

Perhaps the trait should be renamed also to IntoWalletDescriptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes good call, fixed.

CHANGELOG.md Outdated
@@ -27,6 +27,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Wallet
#### Changed
- Removed the explicit `id` argument from `Wallet::add_signer()` since that's now part of `Signer` itself
- Renamed `ToWalletDescriptor::to_wallet_descriptor` to `ToWalletDescriptor::into_wallet_descriptor`
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should mention ToDescriptorKey as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes just added and I also renamed the trait ToDescriptorKey to be IntoDescriptorKey to match the into_descriptor_key() function name.

@tcharding tcharding mentioned this pull request Feb 13, 2021
Merged
9 tasks
afilini
afilini approved these changes Feb 13, 2021
View reviewed changes
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.

utACK 1c6864a

I'm a bit annoyed by this thing that our CI breaks every time there's a new Rust release.. maybe we should consider hard-coding the toolchain version and only manually update it "atomically" in a PR that fixes all the new clippy warnings that come with it.

@afilini afilini merged commit 1c6864a into bitcoindevkit:master Feb 13, 2021
@afilini
Copy link
Member

afilini commented Feb 13, 2021

Merging those into the release branch as well since we'll need the CI to run the tests before publishing

@notmandatory
Copy link
Member Author

utACK 1c6864a
maybe we should consider hard-coding the toolchain version and only manually update it "atomically" in a PR that fixes all the new clippy warnings that come with it.

Ya I agree. I'll create a PR 🙂

@tcharding
Copy link
Contributor

utACK 1c6864a

I'm a bit annoyed by this thing that our CI breaks every time there's a new Rust release.. maybe we should consider hard-coding the toolchain version and only manually update it "atomically" in a PR that fixes all the new clippy warnings that come with it.

Alternatively we can just keep the code linting cleanly using nightly (assuming no breaking changes to previous stable versions) and then this issue shouldn't arise. This PR makes a start.

@notmandatory
Copy link
Member Author

I think trying to keep our code mostly or partially in sync with nightly lint rules will be more work than it's worth.

I'd rather do what @afilini is suggesting above which is to hard code our CI to test against our MSRV 1.45 and the current stable version. We can then upgrade to any newer stable version as they come out with a single PR without CI breaking unexpectedly.

@notmandatory notmandatory mentioned this pull request Feb 15, 2021
Merged
4 tasks
@notmandatory notmandatory deleted the fix/clippy_150 branch September 16, 2021 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcharding tcharding tcharding left review comments

@afilini afilini afilini approved these changes

@LLFourn LLFourn LLFourn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@notmandatory @afilini @tcharding @LLFourn