Skip to content

Implement From<Foo> for Descriptor<Pk> using macro #574

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.

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

Closed

Conversation

tcharding
Copy link
Member

Add a macro and remove all the individual from impls.

Refactor only, no logic changes.

Add a macro and remove all the individual from impls.

Refactor only, no logic changes.
@apoelstra
Copy link
Member

I wish there was a way that you could expand macros prior to committing them. So we could have a CI job that checked that all the code was actually produced from macros, but when reading it, you could read the expanded versions.

In C you can kinda do this by running cpp directly.

In Rust I guess we could also use cpp but that's maybe too evil :).

But basically, I like the guaranteed consistency and ease of writing macros but I really think they harm readability.

@tcharding
Copy link
Member Author

tcharding commented Jul 25, 2023

but I really think they harm readability

The first Rust job I had they barred me from writing macros because of this.

Perhaps we can find a middle ground, if the macro was called in a form more like what the code looks like it would be easier to read. It may even be easier to read because it reduces the noise.

Convert this

impl From<error::Bar> for GeneralError {
    fn from(e: error::Bar) -> Self { Self::Bar(e) }
}
with one of these:
```rust
impl_from_for_error_type!(From<error::Bar> is GeneralError::Bar);
impl_from_for_error_type!(From<error::Bar>, GeneralError::Bar);
impl_from_for_error_type!(error::Bar, GeneralError::Bar);

(I've use an error conversion because of rust-bitcoin/rust-bitcoin#1950 (comment))

@apoelstra
Copy link
Member

Can you use for instead of is? Or does Rust prevent this?

@tcharding
Copy link
Member Author

We can put anything we want in there I believe, I did not code the actual macro up though. (I also had for at one stage when editing the original post)

@tcharding
Copy link
Member Author

tcharding commented Jul 26, 2023

oh that's right, I removed for because its a bit weird to have for and then include the variant name.

@apoelstra
Copy link
Member

Ah, I see. So even in your hypotheticals I misread what was going on ... I think the explicit code is probably better than anything we've got here.

One thing we could experiment with is adding #[from] the way that thiserror does. We've shied away from syntax extensions in the past but possibly this one is easy to do. Unsure. It would probably depend on quote which we've had MSRV trouble with in the past.

@tcharding
Copy link
Member Author

I"ll leave this for now.

@tcharding tcharding closed this Jul 27, 2023
@tcharding tcharding deleted the 07-25-impl-from-descriptor branch August 7, 2023 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants