Skip to content

[#344] Add assert_matches#821

Merged
notmandatory merged 1 commit intobitcoindevkit:masterfrom
Synesso:jem/20221211-assert_matches
Dec 26, 2022
Merged

[#344] Add assert_matches#821
notmandatory merged 1 commit intobitcoindevkit:masterfrom
Synesso:jem/20221211-assert_matches

Conversation

@Synesso
Copy link
Copy Markdown
Contributor

@Synesso Synesso commented Dec 12, 2022

[#344] Add assert_matches

@evanlinjin
Copy link
Copy Markdown
Member

evanlinjin commented Dec 13, 2022

Thank you for the PR. Couldn't we just use assert_eq instead of adding an external dependency?

@danielabrozzoni
Copy link
Copy Markdown
Member

danielabrozzoni commented Dec 13, 2022

Approved the CI run :)

Thank you for the PR. Couldn't we just use assert_eq instead of adding an external dependency?

Note that it's just a dev dependency (it's useful only in tests), so it's not too bad. Also, see: #344 (comment)

@Synesso
Copy link
Copy Markdown
Contributor Author

Synesso commented Dec 13, 2022

Do these failing code cov and doc builds need to pass?

@danielabrozzoni
Copy link
Copy Markdown
Member

danielabrozzoni commented Dec 14, 2022

I merged #822 to fix the CI, try rebasing :)

@Synesso Synesso force-pushed the jem/20221211-assert_matches branch from 7dba289 to 1551088 Compare December 15, 2022 07:57
@thunderbiscuit thunderbiscuit mentioned this pull request Dec 15, 2022
Merged
5 tasks
@notmandatory
Copy link
Copy Markdown
Member

notmandatory commented Dec 16, 2022

I'm all for this change since it's only a dev dependency and assert_matches! is on track to end up in the std lib eventually.

How would you fee about updating the ~29 spots in the code where we use assert!(matches!(...)) to use assert_matches!() instead?

@Synesso Synesso force-pushed the jem/20221211-assert_matches branch from 1551088 to fb43622 Compare December 17, 2022 05:48
@Synesso
Copy link
Copy Markdown
Contributor Author

Synesso commented Dec 17, 2022

Updated @danielabrozzoni @notmandatory PTAL

@Synesso
Copy link
Copy Markdown
Contributor Author

Synesso commented Dec 17, 2022 •
edited
Loading

Pushed a change to fix a linting error (seemed unrelated to my changes).

danielabrozzoni
danielabrozzoni reviewed Dec 19, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

A few nits, but otherwise it's looking really good!

Can you please squash your commits together when you update?

Comment thread src/descriptor/checksum.rs
Comment on lines +159 to +162
assert_matches!(
calc_checksum(desc).err(),
Some(DescriptorError::InvalidDescriptorChecksum)
));
);
Copy link
Copy Markdown
Member

@danielabrozzoni danielabrozzoni Dec 19, 2022

Choose a reason for hiding this comment

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

You can drop the .err() here:

         assert_matches!(
-            calc_checksum(desc).err(),
-            Some(DescriptorError::InvalidDescriptorChecksum)
+            calc_checksum(desc),
+            Err(DescriptorError::InvalidDescriptorChecksum)
         );

and same below

Comment thread src/descriptor/mod.rs Outdated
Comment thread src/wallet/mod.rs Outdated
Error::Signer(SignerError::NonStandardSighash)
),
assert_matches!(
result.unwrap_err(),
Copy link
Copy Markdown
Member

@danielabrozzoni danielabrozzoni Dec 19, 2022

Choose a reason for hiding this comment

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

You can avoid the unwrap_err, and match on Err(Error::Signer(SignerError::NonStandardSighash))

@Synesso Synesso force-pushed the jem/20221211-assert_matches branch from f6b5535 to cb2f0ac Compare December 20, 2022 11:14
@Synesso
Copy link
Copy Markdown
Contributor Author

Synesso commented Dec 20, 2022

Squashed and requested changes applied. TY

danielabrozzoni
danielabrozzoni reviewed Dec 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

There are still some instances of .err() and .unwrap_err() around, can you fix them?

Comment thread src/descriptor/checksum.rs Outdated
@Synesso Synesso force-pushed the jem/20221211-assert_matches branch from cb2f0ac to c04eb7d Compare December 20, 2022 23:34
@rajarshimaitra
Copy link
Copy Markdown
Contributor

rajarshimaitra commented Dec 21, 2022

I approved a run.. Lets see what happens.. :)

@rajarshimaitra
Copy link
Copy Markdown
Contributor

rajarshimaitra commented Dec 21, 2022

Congrats.. All checks passing.. Will do a deeper review tomorrow..

rajarshimaitra
rajarshimaitra previously requested changes Dec 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rajarshimaitra rajarshimaitra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK.. No strong opinion on assert_matches over matches.. Below are few extra changes that I think isn't required..

Comment thread src/testutils/configurable_blockchain_tests.rs Outdated
Comment thread src/testutils/configurable_blockchain_tests.rs Outdated
Comment thread src/testutils/configurable_blockchain_tests.rs Outdated
@Synesso Synesso force-pushed the jem/20221211-assert_matches branch from c04eb7d to 3f3b924 Compare December 23, 2022 11:03
@Synesso
[bitcoindevkit#344] Add assert_matches
14bc9c0 
Replace assert!(matches! with assert_matches! everywhere
Convert assert! to assert_eq! in 2 places
@Synesso Synesso force-pushed the jem/20221211-assert_matches branch from 3f3b924 to 14bc9c0 Compare December 23, 2022 12:06
notmandatory
notmandatory approved these changes Dec 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 14bc9c0

Thanks for updating all the assert!(matches!()).

@notmandatory
Copy link
Copy Markdown
Member

notmandatory commented Dec 24, 2022 •
edited
Loading

@rajarshimaitra if you're OK with the latest push I think you need to resolve your review changes or re-approve or just let me know and I'll go ahead with merging it.

danielabrozzoni
danielabrozzoni approved these changes Dec 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

utACK 14bc9c0

@notmandatory notmandatory dismissed rajarshimaitra’s stale review December 26, 2022 21:40

Since it looks like you are OK with is I have to dismiss the review so Github will let me merge.

@notmandatory notmandatory merged commit 5f0870a into bitcoindevkit:master Dec 26, 2022
@notmandatory notmandatory mentioned this pull request Dec 26, 2022
Closed
@rajarshimaitra
Copy link
Copy Markdown
Contributor

rajarshimaitra commented Dec 27, 2022 •
edited
Loading

Sorry I was AFK for last 3 days.. Made another pass over this and all looks good to me..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@notmandatory notmandatory notmandatory approved these changes

@rajarshimaitra rajarshimaitra Awaiting requested review from rajarshimaitra

+1 more reviewer

@danielabrozzoni danielabrozzoni danielabrozzoni approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Synesso Synesso

Labels

tests

Projects

None yet

Milestone

Release 0.26.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

5 participants

@Synesso @evanlinjin @danielabrozzoni @notmandatory @rajarshimaitra