Skip to content
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

follow up derive_more release 1.0 #55

Closed
yangfengzzz opened this issue Jan 25, 2024 · 7 comments
Closed

follow up derive_more release 1.0 #55

yangfengzzz opened this issue Jan 25, 2024 · 7 comments

Comments

@yangfengzzz
Copy link

please follow derive_more to upgrade 1.0 version.

JelteF/derive_more#259

Because it will use syn 2.0 and a lot other package will use it too. Now in our project, there are two version of syn.

@KillingSpark
Copy link
Owner

Yep it's on my todo list, but if I'm not mistaken 1.0 is not yet released. Right?

@xd009642
Copy link
Contributor

Another potential option would be to remove derive_more and add in the manual implementations. I know it's a bit of faff but from cargo build --timings I can see syn takes up ~25s of my clean build time. Provided there's not too much churn in those types it might be a nicer solution provided someone is willing to go over and write all the boilerplate (I'm willing to PR this if @KillingSpark is okay with the change).

@KillingSpark
Copy link
Owner

I'd accept that, the thought crossed my mind too. I just didn't want to spend the time to actually implement it. If you have the motivation to do this, go for it :)

@mstange
Copy link

mstange commented May 10, 2024

provided someone is willing to go over and write all the boilerplate

Here's a partial attempt I made a few weeks ago: https://github.com/mstange/zstd-rs/commits/displaydoc-instead-of-derive_more/

It uses displaydoc::Display and manually implements From and Error. I initially wanted to use thiserror but then saw that thiserror doesn't support no_std.

After I made the changes linked above, I discovered that thiserror-no-std exists and that you can do this instead:

#[cfg_attr(feature = "std", derive(thiserror::Error))]
#[cfg_attr(not(feature = "std"), derive(thiserror_no_std::Error))]

So I think using thiserror + thiserror-no-std would be even better than what I have in that branch.

@KillingSpark
Copy link
Owner

We actually switched from thiserror_no_std to derive_more. I don't remember why and am on mobile so looking it up is a bit difficult. But I think if this issue is tackled again I would want it to just stay dealt with. Which means either derive_more finally releases 1.0 or we just drop the dependency alltogether. Swapping it just seems like a temporary fix at this point

@mstange
Copy link

mstange commented May 10, 2024

Looking at the history, it was actually thiserror-core that you switched away from, not thiserror-no-std. The former requires Nightly for no_std builds, the latter just doesn't implement any Error trait unless the std feature is used. However, there are other arguments against thiserror-no-std: It hasn't been updated in 2 years, it doesn't have any documentation about how it's different from thiserror, and the package on crates.io still points to dtolnay's repo rather than Stupremee's fork. After finding out those pieces of information, I'm no longer in favor of using thiserror-no-std either.

@xd009642
Copy link
Contributor

Yeah personally I'll just roll the implementations myself, part of my desire is shaving a non-insignificant chunk off my projects build time. Anyway after doing src/frame.rs I only have 169 errors to fix, sounds like a fun evenings work 😅

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

No branches or pull requests

4 participants