Skip to content

hashes: Remove utils module#3299

Merged
apoelstra merged 3 commits intorust-bitcoin:masterfrom
tcharding:09-03-hashes-rename-utils
Oct 29, 2024
Merged

hashes: Remove utils module#3299
apoelstra merged 3 commits intorust-bitcoin:masterfrom
tcharding:09-03-hashes-rename-utils

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Sep 4, 2024

We don't want dump-all module names. Remove utils in favour of macros, while we are at it roll the serde_maros code into it as well and remove that module.

Fix: #2665

@github-actions github-actions bot added the C-hashes PRs modifying the hashes crate label Sep 4, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 4, 2024

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

@github-actions github-actions bot added the API break This PR requires a version bump for the next release label Sep 4, 2024
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Sep 4, 2024

The API break is real, we need to deprecate.

@tcharding tcharding force-pushed the 09-03-hashes-rename-utils branch from e6d8311 to a959c95 Compare September 5, 2024 02:02
@tcharding
Copy link
Copy Markdown
Member Author

I didn't test the deprecation, I remember it not working when I've tried this in the past but it will be interesting to see what the semver bot thinks of it.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Sep 5, 2024

Cool, the bot likes my changes.

Kixunil
Kixunil previously approved these changes Sep 5, 2024
Copy link
Copy Markdown
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 a959c95afb70fa3722e67e62e664c1ee823bcc35

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, this is correct.

@tcharding tcharding force-pushed the 09-03-hashes-rename-utils branch 2 times, most recently from b3b3946 to 6a8072e Compare September 17, 2024 23:20
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Sep 17, 2024

Rebased and improved commit logs, no other changes. Note the range diff has

    - #[cfg(feature = "bitcoin-io")]
    - mod impls;
    + extern crate schemars;
    + 

And that led me to #3374

(How does one get syntax highlighting for diffs? I tried tick-tick-tick followed by diff and by patch)

@apoelstra
Copy link
Copy Markdown
Member

(a) needs rebase again, apparently, and (b) ```diff should work. But I think the stuff you quoted is just too small and headerless to be recognizable as a diff.

@tcharding tcharding force-pushed the 09-03-hashes-rename-utils branch from 6a8072e to 40f56bf Compare September 18, 2024 22:31
@tcharding
Copy link
Copy Markdown
Member Author

#3354 conflicts with this because of the SerdeHash stuff, I rekon that one should go in first. Converting to draft.

@tcharding tcharding marked this pull request as draft September 18, 2024 22:33
@tcharding tcharding force-pushed the 09-03-hashes-rename-utils branch from 40f56bf to 7589d90 Compare October 16, 2024 03:25
@tcharding tcharding marked this pull request as ready for review October 16, 2024 03:26
Copy link
Copy Markdown
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 092d4abb443bb1d9ae8e47d62c002f74c6b483c8; successfully ran local tests

@tcharding
Copy link
Copy Markdown
Member Author

Merge please.

apoelstra
apoelstra previously approved these changes Oct 23, 2024
Copy link
Copy Markdown
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 7589d90b73448132237f9516291a353f65fba3f0; successfully ran local tests; but needs rebase

The private and public modules are already grouped, add a line of
whitespace to make it _even_ more clear. Trivial I know, this patch got
smaller during rebase.
The `utils` module holds public macros, call it `macros` instead.
Roll the `serde_macros` module into `macros`, requires making `macros`
public but since it explicitly holds public macros this is reasonable.

Keep the original module and deprecate it.
@tcharding
Copy link
Copy Markdown
Member Author

Rebased, no other changes.

Copy link
Copy Markdown
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 5a736ed; successfully ran local tests

@apoelstra apoelstra merged commit 0bef101 into rust-bitcoin:master Oct 29, 2024
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 C-hashes PRs modifying the hashes crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hashes::util needs re-naming

3 participants