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

re-export traits together with the derive #271

Closed
JelteF opened this issue Jul 3, 2023 · 6 comments · Fixed by #273
Closed

re-export traits together with the derive #271

JelteF opened this issue Jul 3, 2023 · 6 comments · Fixed by #273
Assignees
Milestone

Comments

@JelteF
Copy link
Owner

JelteF commented Jul 3, 2023

This seems quite nice for traits that are not in the prelude. Often if you're deriving things you also want to use them.

@JelteF JelteF added this to the 1.0.0 milestone Jul 3, 2023
@JelteF
Copy link
Owner Author

JelteF commented Jul 15, 2023

Sadly this is not easy to do without opening ourselves up to unfixable breakage if Rust decides to add some derives for these traits too. This showed up already for the Debug trait. See #273 for details.

@JelteF
Copy link
Owner Author

JelteF commented Jul 15, 2023

So removing this from the 1.0.0 milestone.

@JelteF JelteF removed this from the 1.0.0 milestone Jul 15, 2023
@JelteF
Copy link
Owner Author

JelteF commented Jul 15, 2023

Blocked by trait aliases rust-lang/rust#41517

@tyranron tyranron added this to the 1.0.0 milestone Jul 20, 2023
JelteF added a commit that referenced this issue Jul 23, 2023
Re-export traits from `std` for all our derives. This way people don't
need to import traits manually when they want to reference them
somewhere, such as bounds.

Fixes #271

---------

Co-authored-by: tyranron <[email protected]>
@awused
Copy link

awused commented Aug 14, 2024

This causes a lot of breakages in code that has compiled, unchanged, for years, and there doesn't seem to be any real benefit to it.

Honestly it seemed like such a serious mistake that wasn't even mentioned in the release notes for 1.0.0 that I was halfway done filling out a bug report. I'm surprised it was an intentional change.

edit: I did eventually find it buried under "breaking changes," must've just missed it the first time I read them.

@tyranron
Copy link
Collaborator

tyranron commented Aug 15, 2024

@awused this also is explicitly mentioned and described in the README of the crate both on GitHub and crates.io/docs.rs:

We were quite aware of the unpleasant consequences of this breakage, as had the experience of large code bases migration during 1.0.0-beta.x cycles. That's why, as described in the README, we still left the option to import macros only:

use derive_more::derive::Display; // imports macro only

If you don't have enough time/resources for the full migration, this can ease it just by doing the following replacement in the project:

s/use derive_more::/use derive_more::derive::/

@awused
Copy link

awused commented Aug 15, 2024

Sure, it's mentioned in the readme, but it's not like I re-read every readme on every update. I ran into what I thought was a clear mistake and was about to file a bug, because it didn't seem at all reasonable that one crate would be making decisions on what parts of another crate I wanted to import. Even if that second "crate" is std, it's a bizarre overreach; there's certainly no other crate I can think of that does anything like this, not that many crates would have the opportunity. It leaves a bad taste in my mouth that any library would make that decision for me.

As for the migration, it's not actually about the amount of work required - contrary to the premise of this feature request, it seems quite common to derive a trait without using it, so the actual positive impact of this change is rather negligible. In several of my projects this change had no impact besides breaking compilation in files where I had both a manual definition of a std trait alongside a derive_more derivation.

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 a pull request may close this issue.

3 participants