Skip to content

Remove repetition for hash_newtype#2860

Closed
tcharding wants to merge 1 commit intorust-bitcoin:masterfrom
tcharding:06-12-remove-repetition-from-hash_newtype
Closed

Remove repetition for hash_newtype#2860
tcharding wants to merge 1 commit intorust-bitcoin:masterfrom
tcharding:06-12-remove-repetition-from-hash_newtype

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Jun 12, 2024

Macros are hard enough to work on and read without adding the additional complexity from repetition. Also, since the macro call site is [hopefully] write-once-read-often code making the call site as quick to read as possible is beneficial.

The changes in this patch are subjective, I'd argue that the real benefit is in the future when we work more on refactoring and improving the macro.

@tcharding tcharding marked this pull request as draft June 12, 2024 02:33
@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate test C-base58 PRs modifying the base58 crate labels Jun 12, 2024
@apoelstra
Copy link
Copy Markdown
Member

Yeah, concept ACK. Maaybe we can restore this much later once hashes is at 1.0.

@tcharding
Copy link
Copy Markdown
Member Author

Yeah agreed, for now I think uniformity and ease of patching should be top priority.

Macros are hard enough to work on and read without adding the additional
complexity from repetition. Also, since the macro call site is
[hopefully] write-once-read-often code making the call site as quick to
read as possible is beneficial.

The changes in this patch are subjective, I'd argue that the real
benefit is in the future when we work more on refactoring and improving
the macro.
@tcharding tcharding force-pushed the 06-12-remove-repetition-from-hash_newtype branch from 1a0e937 to 3b5d09c Compare June 17, 2024 02:47
@tcharding tcharding marked this pull request as ready for review June 17, 2024 02:48
@github-actions github-actions bot removed test C-base58 PRs modifying the base58 crate labels Jun 17, 2024
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jun 17, 2024

I'm not against doing it in bitcoin codebase but until the refactor to define engine type is in, I don't see much benefit enforcing it at hashes level. Honestly, this change feels mostly like noise given we have that PR open.

@apoelstra
Copy link
Copy Markdown
Member

I could go either way on enforcing it on the hashes level. Initially I thought it would greatly simplify our macro definitions but actually it's a 2-line diff (on 2 lines that we probably won't ever need to touch).

But I definitely think it's a readablity improvement to do it in bitcoin....and given that it's a definite readability improvement, it seems like it's an API improvement to do it in hashes.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jun 17, 2024

Honestly, I find the untagged ones equally readable. That could be easily because I rewrote the macro, so I'm willing to defer to majority.

@tcharding
Copy link
Copy Markdown
Member Author

I just want to push hashes forward, I'm pretty much finding this crate annoying to work on and have been working on it solidly for what feels like over a month with what feels like fuck all progress.

I'm going to close anything that is contentious.

@tcharding tcharding closed this Jun 19, 2024
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jun 19, 2024

have been working on it solidly for what feels like over a month with what feels like fuck all progress.

Welcome to my world. I had very similar experience previously.

@tcharding tcharding mentioned this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants