8000 Re-export blockdata modules by tcharding · Pull Request #1172 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Re-export blockdata modules #1172

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 2 commits into from
Aug 19, 2022
Merged

Re-export blockdata modules #1172

merged 2 commits into from
Aug 19, 2022

Conversation

tcharding
Copy link
Member

In an effort to make the library more ergonomic to use re-export modules from blockdata at the crate root level. This helps to decouple the internal code layout with the public API.

apoelstra
apoelstra previously approved these changes Aug 8, 2022
View rev 8000 iewed 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 3591314104b8f4025bae649e597ce6cb60910745

@tcharding tcharding added this to the 0.30.0 milestone Aug 11, 2022
@Kixunil
Copy link
Collaborator
Kixunil commented Aug 13, 2022

Concept ACK but maybe also just deprecate the module like we did for util? It'd help when developing not having to type src/blockdata/... every time. :)

@apoelstra
Copy link
Member

I'm ambivalent on removing the actual blockdata/ directory. I agree it'd save typing, but I kinda like having the hierarchy. (Or maybe this is just stockholm syndrome..). I'd ack either way.

@tcharding
Copy link
Member Author
tcharding commented Aug 14, 2022 •
edited
Loading

I had it in mind the that during the tree flattening work the blockdata module would stay. Re-exports were just to decouple the directory structure from the API.

There is also the complexity of constants, we have two modules with this name ( network::constants and blockdata::constants). This PR re-exports blockdata::constants but I guess we need to leave blockdata public so folks can use both modules at once, or is that unlikely?

@tcharding tcharding force-pushed the 08-08-export-blockdata branch from 3591314 to 94a3634 Compare August 14, 2022 22:47
@tcharding
Copy link
Member Author

Rebase only, no other changes.

@Kixunil
Copy link
Collaborator
Kixunil commented Aug 15, 2022

My reasoning is stuff in blockdata is one of the most used in the whole crate. However, now I remembered that I was thinking of making it a separate crate and removing the module would go against that. So I would actually prefer keeping it in for now, until we decide whether we want it separated. I guess I should just cd into the directory when I'm working on it.]

FYI, the reason I'm thinking of separating even things that are not obviously separate is improving the speed of development, mainly skipping irrelevant tests.

apoelstra
apoelstra previously approved these changes Aug 15, 2022
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 94a3634f7c77bbfd96729e0c8b5626c257ac425c

@tcharding
Copy link
Member Author

skipping irrelevant tests

I almost always only run a subset of tests and go on gut feel if I'm breaking other things - seems to be accurate 95% of the time. Would be cool to be able to run thorough tests before pushing PRs though.

sanket1729
sanket1729 previously approved these changes Aug 16, 2022
View reviewed changes
Copy link
Member
@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

utACK 94a3634f7c77bbfd96729e0c8b5626c257ac425c

Kixunil
Kixunil previously approved these changes Aug 16, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 94a3634f7c77bbfd96729e0c8b5626c257ac425c

@tcharding
Re-export blockdata modules
4a119e5 
In an effort to make the library more ergonomic to use re-export modules
from `blockdata` at the crate root level. This helps to decouple the
internal code layout with the public API.
@tcharding tcharding dismissed stale reviews from Kixunil, sanket1729, and apoelstra via 2dd7037 August 16, 2022 23:06
@tcharding tcharding force-pushed the 08-08-export-blockdata branch from 94a3634 to 2dd7037 Compare August 16, 2022 23:06
@tcharding
Copy link
Member Author

Rebase only and fix merge conflicts, on other changes.

@tcharding
Use blockdata exports
6095a4d 
We now export `blockdata` submodules at the crate root level. Use the
new exports for all rustdoc tests/examples.
@tcharding tcharding force-pushed the 08-08-export-blockdata branch from 2dd7037 to 6095a4d Compare August 16, 2022 23:33
@tcharding
Copy link
Member Author

And actually commit the changes.

apoelstra
apoelstra approved these changes Aug 19, 2022
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 6095a4d

sanket1729
sanket1729 approved these changes Aug 19, 2022
View reviewed changes
Copy link
Member
@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 6095a4d

@sanket1729 sanket1729 merged commit 0422359 into rust-bitcoin:master Aug 19, 2022
@tcharding tcharding deleted the 08-08-export-blockdata branch August 22, 2022 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

@Kixunil Kixunil Kixunil left review comments

Assignees
No one assigned
Labels
release notes mention
Projects
None yet
Milestone
0.30.0
Development

Successfully merging this pull request may close these issues.
4 participants
@tcharding @Kixunil @apoelstra @sanket1729
0