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.

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

Edition 2018 #635

Closed
wants to merge 15 commits into from
Closed

Edition 2018 #635

wants to merge 15 commits into from

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Jul 29, 2021

This changes edition to 2018 with a bunch of semi-automated fixes.
I suggest keeping #510 open because this doesn't implement all things mentioned there.

The diff is large but each change should be trivial/obvious.

This is the result of running:
```
cargo +1.36.0 fix --edition --all --broken-code --features="base64,rand,use-serde"
```

And manually fixing [cargo fix
bug](rust-lang/rust#87578)
This changes edition to 2018 and manually fixes a few leftover paths.
All crates currently work with 1.36, so pinning is not needed.
Ran command:
```
cargo +1.36.0 fix --edition-idioms --all --broken-code --features="base64,rand,use-serde"
```

And manually fixed mistakes. Also looks like it didn't strip `::` from
`::core`, so this was done "manually" (regex) too.
Was probably pasted accidentally.
examples/bip32.rs Outdated Show resolved Hide resolved
These were added during conversion to edition 2018.
thomaseizinger
thomaseizinger previously approved these changes Aug 2, 2021
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for pushing this forward!

I think to merge, @apoelstra wants every commit to pass CI so we will need to squash almost all of this together into one.

It is a noisy diff but easy to to go through. Concept ACK from my side. As done here, only the minimally needed changes should be good to merge this. Refinements can be done in follow-ups!

@Kixunil
Copy link
Collaborator Author

Kixunil commented Aug 3, 2021

I intend to squash it all into one commit as it'd be quite annoying to rewrite history. (I know how to do it and I did it several times but in this case I expect it to be too much work.) Does anyone wish to keep some steps?

dr-orlovsky
dr-orlovsky previously approved these changes Aug 9, 2021
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. The amount of painstaking work here is very impressive. I do confirm that these changes are nothing else than just

  • Change of :: prefixes to crate::
  • Removal of unnecessary extern crate imports
  • Addition of lifetime placeholders (<'_>) where they are required

I left few Nits and further optimization questions since this will require squash anyway.

All other improvements related to 2018 edition should be clearly done with a separate PRs in very small sets, otherwise it will take forever to review and merge.

@@ -10,15 +10,10 @@ fi

pin_common_verions() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think we can rename this into cleanup

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking of keeping it if some dependencies update their MSRV. But not really sure if it's better than renaming.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/network/message_blockdata.rs Show resolved Hide resolved
src/util/psbt/macros.rs Outdated Show resolved Hide resolved
@dr-orlovsky dr-orlovsky added the API break This PR requires a version bump for the next release label Aug 9, 2021
This removes needless `extern crate` and changes `pub use x` to
`pub extern crate x` which is more clear in its intent.
@Kixunil
Copy link
Collaborator Author

Kixunil commented Aug 10, 2021

@dr-orlovsky resolved nits except the naming which I'm unsure about which semantics we want to keep.

Thankfully it was not that painstaking - despite its flaws cargo fix is quite handy. :)

The `test` crate is provided by the compiler and seems to be exceptional
in needing `extern crate test` declaration.
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, will do a thorough tests later

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.

Few more comments after line-checking diff with the previous version

impl str::FromStr for SigHashType {
impl core::str::FromStr for SigHashType {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How this was working with no_std before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No clue, I was surprised as well.

src/lib.rs Show resolved Hide resolved
@apoelstra
Copy link
Member

concept ACK. I'd like to first get a breaking version out that implements Taproot and nothing else, which could mean rebasing this several times, unfortunately. But the alternative is probably to rebase every single other PR, and we're running out of pre-Taproot time..

I'm willing to make an exception to the "every commit passes" rule for something like this.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Sep 9, 2021

I'm fine with Taproot and nothing else approach. I didn't have much time for this lately anyway.

@Kixunil Kixunil added this to the 0.29.0 milestone Nov 18, 2021
@tcharding tcharding mentioned this pull request Nov 25, 2021
@tcharding
Copy link
Member

tcharding commented Apr 26, 2022

Are you planning on updating this PR @Kixunil? All the 'before edition bump' PRs are ack'ed and ready to merge. Don't mind me, we need those to merge first, right, that was the point :)

@sanket1729
Copy link
Member

sanket1729 commented Apr 30, 2022

@Kixunil Everything with the label has been resolved. We can now proceed with this PR.

@Kixunil
Copy link
Collaborator Author

Kixunil commented May 2, 2022

OK, I expect this week to be intense for me. If anyone has the time you can overtake. If not, I will try next week or even this one if I get lucky.

@tcharding tcharding mentioned this pull request May 2, 2022
@tcharding
Copy link
Member

If anyone has the time you can overtake.

Done, thanks for blessing me to do so. #983.

I'm going to close this so as folk don't get confused.

@tcharding tcharding closed this May 2, 2022
apoelstra added a commit that referenced this pull request May 18, 2022
9f0c687 Enable edition 2018 (Tobin C. Harding)
dca0d67 Fix in preparation for next edition (Tobin C. Harding)

Pull request description:

  This PR supersedes #635 at the permission of @Kixunil in the thread of that PR.

  Do a minimal set of changes to enable edition 2018.  Patch 1 is  the biggest change set, it is done with `cargo`, no other manual changes are included in patch 1.  It can verified by running `cargo fix --edition` on master and checking the diffs are the same.

  Patch 2 enables 2018 and includes all the manual changes required to get the code to build (with _all_ the feature combinations :)

ACKs for top commit:
  dunxen:
    re-ACK  9f0c687
  apoelstra:
    re-ACK 9f0c687
  RCasatta:
    ACK 9f0c687,

Tree-SHA512: 7c23554adb4c1dd932af1e80f04397bad0418b4fdae2d0fe243c3f19ba1169686a386bffd38a3c02871c7544b5a7bc8af525b50617a2695726408c091700f081
@Kixunil Kixunil deleted the edition-2018 branch May 24, 2022 09:38
@Kixunil Kixunil removed this from the 0.29.0 milestone Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API break This PR requires a version bump for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants