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

Stop exporting traits by default and early 2.0 release #405

Closed
JelteF opened this issue Aug 29, 2024 · 3 comments · Fixed by #406
Closed

Stop exporting traits by default and early 2.0 release #405

JelteF opened this issue Aug 29, 2024 · 3 comments · Fixed by #406
Assignees
Milestone

Comments

@JelteF
Copy link
Owner

JelteF commented Aug 29, 2024

The amount of opened issues and negative feedback raised because we export traits together with derives by default in 1.0 is much larger than I had expected. So I'm left wondering if we made the right decision here, and if the downsides are worth the upsides.

The downsides I'm seeing are:

  1. Confusing people that add derive_more to an existing codebase with strange import conflict errors
  2. Conflicting with people calling their types Error
  3. Causing people upgrading from 0.99 extra breaking changes

Benefits:

  1. No need to import the trait if you use it

I think the following reddit comment made good points on why this is the possibly the wrong tradeoff. tl;dr people familiar with rust are used to the errors of not having imported an std trait, and rust-analyzer helps solve these issues easily. But these conflicting import errors have are uncommon, and thus a hassle to solve especially because there's no tooling to fix them automatically.

I have to agree, reexporting std here is the baaaad kind of efficiency. You're creating unexpected, non-standard behavior. And behavior that existing tooling (errors, lints, etc.) does not handle gracefully (because it's abnormal). That's almost always bad, and creates invisible debt and the worst kind of work (fake work trying to deal with obtuse, non-substantive errors). And in this case the only plus is that it removes the need to do something rust analyzer and an ide would almost automate anyway. I love the derive_more crate, but I agree that this is the bad kind of complexity. (e.g. I expect to see use std:... if I'm using it) Any introduction of unexpected behavior needs to solve a serious problem. An example of headache: I was just writing some exploratory code using the new derive_more and also just spent time trying to figure out where naming collisions were coming from. It doesn't matter that the "finished" code wouldn't have those naming collisions. When I'm in the process of writing code I'm going to explore things based on assumptions about the state of imports etc in a file/crate. (Perhaps there's a large upside that I'm not seeing, but this mostly seems to be doing the work that tooling already does in a way that's opaque. Also, this probably seems trivial to you because you're intimately familiar with your own crate. Almost no one else is going to expect this, creating headache whenever it comes up. Edge case or otherwise.)
——

Edit: I love your crate and am super thankful for it!!! Just to be clear. (Realized that sentiment may be lost in the minutae discussion.)

Source: https://www.reddit.com/r/rust/comments/1emi4fi/comment/lhkmjpk/

Right now I'm definitely leaning towards the feeling of: "If I could go back in time, I think we should not have made this change".

The question then becomes, how can we make changing this back as painless as possible, now that we have made this "mistake".

The best approach I can see is by doing the following:

  1. Create a 2.0 release. To not cause silent breakage for people already on 1.0.
  2. Create a module like derive_more::both or derive_more::trait_derive that people already on 1.0 can use.
  3. Do this relatively quickly, while 1.0 is not used a lot yet.

Related to:

@tyranron
Copy link
Collaborator

@JelteF I do still lean towards the feedback I was giving during 1.0.0-beta.* cycles and consider this as unpleasant breakage, yet leading to the better code. Most of the people's complaints are just a result of their inertia to adapt and getting used to old ways, imho. Allowing to mix up Error trait with Error types in the same namespace is not good and hides the problem, which reappears just there whenever you need to implement Error by hand for some reason. This also gives additional pain with intra-doc links, as requires to disambiguate Error name in docs whenever you use it. Disallowing those names to collide stimulates user to name their errors better (in our refactoring a whole bunch of Errors was renamed into more meaningful ParseError or similar).

However, it seems that I'm in minority here.

  1. Create a module like derive_more::both or derive_more::trait_derive that people already on 1.0 can use.

I'm totally OK if we swap them. The name I propose derive_more::with_trait. I'll continue using imports derive_more::with_trait in my projects, yet people will be less confused.

  1. Create a 2.0 release. To not cause silent breakage for people already on 1.0.

This is also the case to address the problem with mixed #[display()] attributes. So I do agree here too.

@JelteF
Copy link
Owner Author

JelteF commented Aug 29, 2024

Allowing to mix up Error trait with Error types in the same namespace is not good and hides the problem, which reappears just there whenever you need to implement Error by hand for some reason.

I do agree with you that it's an anti-pattern, but I don't think it's derive_more it's place to get people to avoid this anti-pattern. Also as long as you fully qualify the error trait it's not a big problem without derive_more, but as soon as you then import the Error derive you either have to use use derive_more::Error as ErrorTrait or use #[derive(derive_more::Error)] to avoid this problem. Neither of which were necessary with 0.99.

I'm totally OK if we swap them. The name I propose derive_more::with_trait. I'll continue using imports derive_more::with_trait in my projects, yet people will be less confused.

Alright, lets do this then. And indeed with_trait sounds like a good submodule.

@awused
Copy link

awused commented Sep 6, 2024

As an outsider, I really question benefit of even including a with_trait submodule. I do not think it will see much usage. The vast majority of the traits in derive_more do not need to be explicitly imported when used: Debug/Derive are usually used through format macros, Add/Subtract/etc use operators, From uses .into(), Deref uses *, etc. I think the benefits were greatly overstated or at least the sample project was very atypical.

Across all my projects, both public and private, the change to 1.0.0 did not save even a single import line where trait methods were being used. It did save a couple (<10) of lines where I had both manual and derived implementations of the same trait in the same file, but that was fairly uncommon. I would not choose to import derive_more::with_trait::Trait in those cases with 2.0.0, and with 1.0.0 I opted to replace all derive_more:: imports with derive_more::derive:: rather than save the couple of imports.

tyranron added a commit that referenced this issue Sep 9, 2024
As was decided in #405, the crate's root should re-export only macros.
Re-exporting of macros along with their corresponding traits should be
moved to `with_trait` module.

Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants