Fixed a bunch of clippy lints, added clippy.toml by Kixunil · Pull Request #686 · rust-bitcoin/rust-bitcoin

Kixunil · 2021-11-03T10:52:38Z

This is the initial step towards using and maybe enforcing clippy.
It does not fix all lints as some are not applicable. They may be
explicitly ignored later.

Some discussion about clippy was in #685

sanket1729

ACK fea88aea33a9ac2d03a77cf08eade0ebdad2da01

tcharding

Nice one, thanks.

src/blockdata/constants.rs

src/network/address.rs

tcharding · 2021-11-04T22:19:15Z

This is the initial step towards using and maybe enforcing clippy.

Of possible relevance to this work is: #688 (further configures clippy).

DON-MAC-256

Nice changes, including some that should help prevent the introduction of bugs later on.

ACK

dr-orlovsky

LGTM mod two nits

dr-orlovsky · 2021-11-06T20:47:45Z

src/blockdata/transaction.rs

#[derive(ParialEq, Eq, PartialOrd, Ord)], according to our new contributing guidelines :)

TBH, I'm not really convinced errors should commit to Ord/PartialOrd. Maybe Eq is fine.

What about Hash?

If there's Eq there should be Hash, I believe.

In this specific case I'm not even sure about Eq. Are two errors carrying information saying "can't recognize sighash string" (by their existence) that were caused by different input (which they write for ease of debugging) equal or not?

dr-orlovsky · 2021-11-06T20:49:08Z

src/network/message.rs

Here I still think #[derive(PartialEq, Eq)] will be nice to have

dr-orlovsky · 2021-11-23T16:43:13Z

@Kixunil needs rebase :(

Kixunil · 2021-11-23T17:03:56Z

Rebased

dr-orlovsky

ACK 693f6666da64bf3140d50ca9983f3968c4c69802 with one small nit

dr-orlovsky · 2021-11-23T17:07:58Z

src/network/message.rs

Here I still think #[derive(PartialEq, Eq)] will be nice to have

Kixunil · 2021-11-23T17:14:23Z

Will wait a bit if others have an opinion on this. Basically the question is "Do we want to commit to always carry error information that can be meaningfully compared?"

RCasatta · 2021-11-23T17:19:16Z

If I remember correctly @apoelstra doesn't like struct initialization shorthand

Kixunil · 2021-11-23T17:25:29Z

Shame, I do like it but willing to allow it instead.

tcharding · 2021-11-24T00:24:20Z

Adding allows everywhere will be messy as hell, structs get initialized all over the place. Probably best to let @apoelstra comment before starting a fight in his name though :)

Kixunil · 2021-11-24T07:07:32Z

@tcharding I think they could be allowed globally in clippy.toml

tcharding · 2021-11-24T21:32:43Z

yeah, I realised this last night when I was going to sleep :)

apoelstra · 2021-12-21T15:20:42Z

Sorry for being absent.

I don't like the compact struct initialization format but I won't hold things up about it. I'm used to it by now :).

Regarding errors satisfying PartialEq, Eq etc ... I tend to agree that they shouldn't be comparable. If you need to nest them in structs then you can wrap them or something.

Needs rebase.

This is the initial step towards using and maybe enforcing clippy. It does not fix all lints as some are not applicable. They may be explicitly ignored later.

Kixunil · 2021-12-21T21:52:04Z

Rebased

apoelstra

ACK 779d411

The only thing that raised my eyebrows was the initialization thing, but I'll choose some other hill to die on.

RCasatta

ACK 779d411

Kixunil mentioned this pull request Nov 3, 2021

Could we get a contribution/style guideline and Makefile? #685

Closed

Kixunil force-pushed the clippy branch 2 times, most recently from e70297b to fea88ae Compare November 3, 2021 11:49

sanket1729 previously approved these changes Nov 3, 2021

View reviewed changes

tcharding reviewed Nov 4, 2021

View reviewed changes

src/blockdata/constants.rs Outdated Show resolved Hide resolved

src/network/address.rs Outdated Show resolved Hide resolved

DON-MAC-256 previously approved these changes Nov 8, 2021

View reviewed changes

dr-orlovsky previously approved these changes Nov 11, 2021

View reviewed changes

Kixunil dismissed stale reviews from dr-orlovsky, DON-MAC-256, and sanket1729 via 693f666 November 23, 2021 17:03

Kixunil force-pushed the clippy branch from fea88ae to 693f666 Compare November 23, 2021 17:03

dr-orlovsky previously approved these changes Nov 23, 2021

View reviewed changes

src/network/message.rs Outdated

Copy link
Copy Markdown

Collaborator

dr-orlovsky Nov 23, 2021

Choose a reason for hiding this comment

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

Here I still think #[derive(PartialEq, Eq)] will be nice to have

Kixunil added code quality Makes code easier to understand and less likely to lead to problems help wanted labels Nov 23, 2021

Kixunil added the one ack PRs that have one ACK, so one more can progress them label Dec 2, 2021

Fixed a bunch of clippy lints, added clippy.toml

779d411

This is the initial step towards using and maybe enforcing clippy. It does not fix all lints as some are not applicable. They may be explicitly ignored later.

Kixunil dismissed dr-orlovsky’s stale review via 779d411 December 21, 2021 21:51

Kixunil force-pushed the clippy branch from 693f666 to 779d411 Compare December 21, 2021 21:51

Kixunil removed the one ack PRs that have one ACK, so one more can progress them label Dec 21, 2021

Kixunil requested review from dr-orlovsky and sanket1729 December 21, 2021 21:52

apoelstra approved these changes Dec 22, 2021

View reviewed changes

Kixunil added the one ack PRs that have one ACK, so one more can progress them label Dec 23, 2021

RCasatta approved these changes Dec 24, 2021

View reviewed changes

RCasatta merged commit f9b3fc9 into rust-bitcoin:master Dec 24, 2021

Kixunil deleted the clippy branch December 24, 2021 09:04

sanket1729 mentioned this pull request Dec 24, 2021

Add support for taproot psbt fields BIP 371 #681

Merged

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Conversation

Kixunil commented Nov 3, 2021

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tcharding commented Nov 4, 2021

Uh oh!

DON-MAC-256 left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Nov 6, 2021

Choose a reason for hiding this comment

Uh oh!

Kixunil Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

Kixunil Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Nov 6, 2021

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Nov 23, 2021

Uh oh!

Kixunil commented Nov 23, 2021

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Nov 23, 2021

Uh oh!

RCasatta commented Nov 23, 2021

Uh oh!

Kixunil commented Nov 23, 2021

Uh oh!

tcharding commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Nov 24, 2021

Uh oh!

tcharding commented Nov 24, 2021

Uh oh!

apoelstra commented Dec 21, 2021

Uh oh!

Kixunil commented Dec 21, 2021

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

RCasatta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tcharding commented Nov 24, 2021 •

edited

Loading