Skip to content

Enable edition 2018 #222

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
Merged
merged 4 commits into from
Aug 17, 2022
Merged

Enable edition 2018 #222

merged 4 commits into from
Aug 17, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented May 18, 2022 •
edited
Loading

The rust-bitcoin stack is moving to a MSRV of 1.41.1

Do a [reasonably minimal] patch set that enables editon 2018

  • Patch 1 - Preparatory cleanup
  • Patch 2 - Update CI, README, and test script
  • Patch 3 - Run cargo fix --edition
  • Patch 4 - Enable edition 2018

@tcharding
Run cargo fmt
b898243 
For some reason `integration_test/src/main.rs` has formatting issues, no
clue how this got past CI.

Run `cargo fmt`.
@tcharding tcharding changed the title 05 18 edition 2018 Enable edition 2018 May 18, 2022
@tcharding tcharding force-pushed the 05-18-edition-2018 branch from 5bca083 to aa4e6df Compare May 18, 2022 05:27
dunxen
dunxen approved these changes May 30, 2022
View reviewed changes
Copy link

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

cr-ACK aa4e6df

All patches look good to me!

@tcharding
Copy link
Member Author

tcharding commented May 30, 2022 •
edited
Loading

What is a cr-ACK please? Is it 'code-review' implying that the review is just for code style etc. and not for correctness of the logic?

@dunxen
Copy link

dunxen commented May 31, 2022

What is a cr-ACK please? Is it 'code-review' implying that the review is just for code style etc. and not for correctness of the logic?

Full code review but haven't tested. Basically untested ACK. I do agree in concept too, though.

@tcharding tcharding mentioned this pull request Aug 8, 2022
Merged
@tcharding
Copy link
Member Author

Friendly bump, can we get some review on this one please. The rest of the stack has increased the MSRV so we need to do it here if we want to use the latest versions of crates lower down our stack.

@tcharding
Copy link
Member Author

I just noticed a few formatting changes have snuck into this PR, I can remove them if folks would like me to.

@dunxen
Copy link

dunxen commented Aug 9, 2022

I just noticed a few formatting changes have snuck into this PR, I can remove them if folks would like me to.

Yes please, if it's not too much trouble :)

Still looks good to me though!

@tcharding
Copy link
Member Author

Duh, I'm a wombat - there are formatting changes in this PR because the first patch is a formatting patch.

Changes in force push:

  • ran formatter on patch 3 1a666bd Fix in preparation for next edition

@dunxen
Copy link

dunxen commented Aug 15, 2022

Ah, got it! Looks good, but I think GitHub Actions is not happy with the missing env for rust: 1.41.1. At least that's the only possible reason I can see it complaining about empty string...

Sorry for the delays. You can go ahead and ping me for review again :)

@cryptoquick cryptoquick mentioned this pull request Aug 15, 2022
Closed
dunxen
dunxen approved these changes Aug 16, 2022
View reviewed changes
Copy link

@dunxen dunxen 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 dbc5396

LGTM! Think we can squash now.

@tcharding tcharding mentioned this pull request Aug 16, 2022
Closed
2 tasks
@tcharding
Copy link
Member Author

Think we can squash now.

How did you want them squashed, everything into a single commit?

@tcharding
Remove 1.29 restriction from CI and README
4e888de 
The rust-bitcoin stack is moving towards an MSRV of 1.41.1

In preparation for updating our MSRV from 1.29 to 1.41.1 update the
README, test script, and CI pipeline to reflect the change.
@tcharding
Fix in preparation for next edition
49db004 
Use cargo to upgrade from edition 2015 to edition 2018.

cargo fix --edition

No manual changes made. The result of the command above is just to fix
all the use statements by adding `crate::`.
@tcharding
Enable edition 2018
18d14b6 
Add `edition = "2018"` to each of the three subcrate manifest files.
Fix `::serde` imports and remove some of the unnecessary instances of
`extern crate Foo`.
@tcharding
Copy link
Member Author

I squashed what was the last commit into commit 2 (the RUSTFMTCHECK change). Each commit is now discreet and passes CI. Ok like that?

@stevenroose stevenroose merged commit c861370 into rust-bitcoin:master Aug 17, 2022
@stevenroose
Copy link
Collaborator

Nice, thanks for leaving the rustfmt stuff (mostly) out!

@tcharding tcharding deleted the 05-18-edition-2018 branch August 17, 2022 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunxen dunxen dunxen 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.
3 participants
@tcharding @dunxen @stevenroose