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

Rearrange all the modules #525

Closed
wants to merge 10 commits into from
Closed

Rearrange all the modules #525

wants to merge 10 commits into from

Conversation

apoelstra
Copy link
Member

Alternate/more agressive version of #518

This is a very invasive change but it's something I've wanted to do for years. Curious what others think.

@apoelstra apoelstra added the API break This PR requires a version bump for the next release label Nov 26, 2020
@dpc
Copy link
Contributor

dpc commented Nov 26, 2020

Anything flattening the namespaces is a great improvement.

@stevenroose
Copy link
Collaborator

Hmm yeah this is fine. As for network::VersionMessage vs network::message::Version, I'm rather indifferent. Maybe slightly towards the former as in the latter, the network::XXX namespace is quite empty, though. It has Network (but that one is exposed as bitcoin::Network as well), ServiceFlags and Address.

Perhaps rename mod message; to mod msg;?

@stevenroose
Copy link
Collaborator

but yeah utACK d7389df (but slight preference to make message full names slightly shorter).

@apoelstra
Copy link
Member Author

apoelstra commented Nov 27, 2020 •
edited

That's fair. I should put VersionMessage back because Version by itself is easy to confuse with service flags.

I'm also happy to rename message to msg. and primitives to prim. What do others think?

@apoelstra
Copy link
Member Author

Regarding primitives ... aside from the constants I don't expect people will be using primitives:: paths directly since all of the actual primitives should be re-exposed at the root. So maybe it is okay to have a long name.

@apoelstra
Copy link
Member Author

Let's bundle this with #494 in a "dramatically break the API but don't change functionality" release after the next release.

@justinmoon
Copy link

Anything flattening the namespaces is a great improvement.

What do people think about getting rid of the util module? Half of our code lives in there ...

@apoelstra
Copy link
Member Author

Yeah, I think it might be reasonable to make util private and re-export all of its submodules at the root.

@RCasatta
Copy link
Collaborator

Let's bundle this with #494 in a "dramatically break the API but don't change functionality" release after the next release.

Maybe we can take the occasion for cargo fmt?

@dr-orlovsky
Copy link
Collaborator

Yeah, I was looking for something like this for a long time!!!

Few more suggestions of what can be bundled with this huge breaking change release:

  1. Agree with @RCasatta above on rust fmt. And, it's review may be very much simplified by the reviewers if they will just checkout commit before rust fmt, apply fmt and make sure the result match the committed version. No need to review a plenty of code in this case.
  2. What about adding bitcoin_num migration [epic] Tracking issue for bitcoin_num adoption #467 ?
  3. Such things as bitcoin::amount::Amount and bitcoin:address::Address does not make much sense, if we have bitcoin::PublicKey and not bitcoin::key::PublicKey.

@apoelstra
Copy link
Member Author

@dr-orlovsky I'm definitely in favor of reexporting Amount and Address at the top level if they aren't already.

rustfmt is still really invasive. If there were a way we could phase it in over multiple commits or even multiple PRs I'd be much more willing to take it on.

@stevenroose stevenroose mentioned this pull request Dec 15, 2020
Closed
@apoelstra apoelstra added this to the 0.27.0 milestone Jan 11, 2021
@thomaseizinger thomaseizinger mentioned this pull request Apr 13, 2021
Merged
@RCasatta RCasatta mentioned this pull request Aug 5, 2021
Closed
@dr-orlovsky dr-orlovsky modified the milestones: 0.28.0, 0.29.0 Sep 17, 2021
@tcharding
Copy link
Member

If we are bumping MSRV soon and considering enabling rustfmt (#959) do we want to revisit this at the same time?

@apoelstra
Copy link
Member Author

Yes -- though let's do a MSRV-only release first.

@Kixunil Kixunil modified the milestones: 0.29.0, 0.30.0 Apr 25, 2022
This was referenced Sep 20, 2022
Merged
Closed
@tcharding
Copy link
Member

I've started to bring this back to life:

If these two are well received I can do patches 4-6 as well.

@apoelstra
Copy link
Member Author

Probably we should close this specific PR though?

@tcharding
Copy link
Member

Closing this, will re-do the network message stuff.

@tcharding tcharding closed this Sep 20, 2022
apoelstra added a commit that referenced this pull request Oct 24, 2022
@apoelstra
Merge #1295: Remove usage of opcodes::all
afcdfa4 
c3e4399 Remove usage of opcodes::all (Tobin C. Harding)

Pull request description:

  We have all of the opcodes defined in a submodule called `all`, this allows wildcard imports without bringing in the other types in the `opcodes` module.

  Use wildcard import `use crate::blockdata::opcodes::all::*` instead of fully qualifying the path to opcodes.

  ### Original PR description (left here so the thread of discussion below makes sense)

  The `all` module adds no value, we can remove it without loss of meaning or clarity, doing so makes the code less verbose.

  EDIT: After review, now includes importing with wildcard and removing the `opcodes::` path from any type starting with `OP_`.

  Idea stolen from: #525 (patch 7)

ACKs for top commit:
  Kixunil:
    ACK c3e4399
  apoelstra:
    ACK c3e4399

Tree-SHA512: 300511d909a25e82c563b18da1b52bcf65653cd3efd8ff32dd5b9e839dacd57924953c1745dfb5e9301fa4f9fc0cd61a075f3a3fd94f6a5a9730bca5155dfd96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
API break This PR requires a version bump for the next release
Projects
None yet
Milestone
0.30.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@apoelstra @dpc @stevenroose @justinmoon @RCasatta @dr-orlovsky @tcharding @Kixunil