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

Small modernization after MSRV bump #482

Merged
merged 5 commits into from Oct 9, 2020
Merged

Small modernization after MSRV bump #482

merged 5 commits into from Oct 9, 2020

Conversation

elichai
Copy link
Member

@elichai elichai commented Sep 14, 2020

This is akin to rust-bitcoin/rust-secp256k1#232
but also finally really removed the hex dependency from the tests (I have no idea how did I miss it in #381 )

dpc
dpc previously approved these changes Sep 14, 2020
View reviewed changes
dr-orlovsky
dr-orlovsky previously approved these changes Sep 18, 2020
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Two small notes, otherwise tACK bf984ff

src/internal_macros.rs Outdated Show resolved Hide resolved
src/lib.rs
@@ -75,8 +61,6 @@ pub mod network;
pub mod blockdata;
pub mod util;
pub mod consensus;
// Do not remove: required in order to get hash types implementation macros to work correctly
#[allow(unused_imports)]
pub mod hash_types;

pub use hash_types::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good as a part of this syntax PR to get rid of remaining * imports like this and specify all types explicitly

sgeisler
sgeisler previously approved these changes Sep 18, 2020
View reviewed changes
@elichai elichai dismissed stale reviews from sgeisler, dr-orlovsky, and dpc via 75f74fc September 21, 2020 11:38
@elichai
Copy link
Member Author

elichai commented Sep 21, 2020

Fixed @dr-orlovsky comment, @sgeisler @dpc it dismissed your reviews, sorry about that

dr-orlovsky
dr-orlovsky reviewed Sep 21, 2020
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Error::cause is also deprecated since since 1.33.0, even before description - why we still keep it and do not mart with #[allow_deprecated]?

src/lib.rs
@@ -75,8 +61,6 @@ pub mod network;
pub mod blockdata;
pub mod util;
pub mod consensus;
// Do not remove: required in order to get hash types implementation macros to work correctly
#[allow(unused_imports)]
pub mod hash_types;

pub use hash_types::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub use hash_types::*;
pub use hash_types::{Txid, Wtxid, BlockHash, SigHash, PubkeyHash, WPubkeyHash, ScriptHash, WScriptHash, TxMerkleNode, WitnessMerkleNode, WitnessCommitment, XpubIdentifier, FilterHash};

@elichai
Copy link
Member Author

elichai commented Sep 21, 2020

Error::cause is also deprecated since since 1.33.0, even before description - why we still keep it and do not mart with #[allow_deprecated]?

That is definitely weird, I don't see any deprecation warnings

@RCasatta
Copy link
Collaborator

RCasatta commented Oct 2, 2020

maybe this could remove also #![allow(ellipsis_inclusive_range_patterns)] in lib.rs and maybe review other attributes like #![cfg_attr(feature = "clippy", allow(extend_from_slice))] // extend_from_slice only available since 1.6

@elichai
Copy link
Member Author

elichai commented Oct 4, 2020

maybe this could remove also #![allow(ellipsis_inclusive_range_patterns)] in lib.rs and maybe review other attributes like #![cfg_attr(feature = "clippy", allow(extend_from_slice))] // extend_from_slice only available since 1.6

Already done that: https://github.com/rust-bitcoin/rust-bitcoin/pull/482/files#diff-b4aea3e418ccdb71239b96952d9cddb6

@RCasatta
Copy link
Collaborator

RCasatta commented Oct 4, 2020

maybe this could remove also #![allow(ellipsis_inclusive_range_patterns)] in lib.rs and maybe review other attributes like #![cfg_attr(feature = "clippy", allow(extend_from_slice))] // extend_from_slice only available since 1.6

Already done that: https://github.com/rust-bitcoin/rust-bitcoin/pull/482/files#diff-b4aea3e418ccdb71239b96952d9cddb6

Missed that, much sorry

@Kixunil
Copy link
Collaborator

Kixunil commented Oct 4, 2020

That is definitely weird, I don't see any deprecation warnings

It was soft-deprecated (no warnings, just doc) for a long time (since source() was introduced, I think). It's truly deprecated now, can't remember which version.

Just using Display and source() is really the most sensible approach. FYI Display should not print out the content of source() because users may want to reformat it (e.g. put each source on a separate line).

@RCasatta
Copy link
Collaborator

RCasatta commented Oct 5, 2020

@elichai you may want to pick bb3171b here?

stevenroose
stevenroose previously approved these changes Oct 8, 2020
View reviewed changes
Copy link
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

ACK

apoelstra
apoelstra previously approved these changes Oct 8, 2020
View reviewed changes
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 75f74fc

dr-orlovsky
dr-orlovsky approved these changes Oct 8, 2020
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK

apoelstra
apoelstra approved these changes Oct 8, 2020
View reviewed changes
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 eda47c3

@stevenroose stevenroose merged commit 5355b36 into master Oct 9, 2020
@elichai elichai deleted the 2020-09-modern branch October 10, 2020 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

@stevenroose stevenroose stevenroose approved these changes

@apoelstra apoelstra apoelstra approved these changes

@dpc dpc dpc left review comments

@sgeisler sgeisler sgeisler left review comments

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
8 participants
@elichai @RCasatta @Kixunil @dpc @dr-orlovsky @stevenroose @apoelstra @sgeisler