Skip to content

Rearrange all the modules#525

Closed
apoelstra wants to merge 10 commits intorust-bitcoin:masterfrom
apoelstra:2020-11--redo-modules
Closed

Rearrange all the modules#525
apoelstra wants to merge 10 commits intorust-bitcoin:masterfrom
apoelstra:2020-11--redo-modules

Conversation

@apoelstra
Copy link
Copy Markdown
Member

@apoelstra apoelstra commented Nov 26, 2020

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
Copy Markdown
Contributor

dpc commented Nov 26, 2020

Anything flattening the namespaces is a great improvement.

@stevenroose
Copy link
Copy Markdown
Collaborator

stevenroose commented Nov 27, 2020

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
Copy Markdown
Collaborator

stevenroose commented Nov 27, 2020

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

@apoelstra
Copy link
Copy Markdown
Member Author

apoelstra commented Nov 27, 2020 •
edited
Loading

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
Copy Markdown
Member Author

apoelstra commented Nov 27, 2020

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
Copy Markdown
Member Author

apoelstra commented Nov 27, 2020

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
Copy Markdown

justinmoon commented Nov 27, 2020

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
Copy Markdown
Member Author

apoelstra commented Nov 27, 2020

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

@RCasatta
Copy link
Copy Markdown
Collaborator

RCasatta commented Nov 28, 2020

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?

@apoelstra apoelstra force-pushed the 2020-11--redo-modules branch from 0643aad to 6f71797 Compare December 3, 2020 02:46
@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Dec 5, 2020

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
Copy Markdown
Member Author

apoelstra commented Dec 6, 2020

@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
Copy Markdown
Member

tcharding commented Apr 20, 2022

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

@apoelstra
Copy link
Copy Markdown
Member Author

apoelstra commented Apr 20, 2022

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
Copy Markdown
Member

tcharding commented Sep 20, 2022

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
Copy Markdown
Member Author

apoelstra commented Sep 20, 2022

Probably we should close this specific PR though?

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Sep 20, 2022

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.

8 participants

@apoelstra @dpc @stevenroose @justinmoon @RCasatta @dr-orlovsky @tcharding @Kixunil