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 #273

Merged
merged 11 commits into from
Jul 23, 2023
Merged

Re-export traits #273

merged 11 commits into from
Jul 23, 2023

Conversation

JelteF
Copy link
Owner

@JelteF JelteF commented Jul 15, 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

This is blocked until [trait aliases are allowed][1]. Because right new we'd
also export any derives from std with the same name. This showed up with
the Debug derive. But if rust starts implementing more derives for their
built in types in std, then the same issue would occur for other traits.

[1]: rust-lang/rust#41517
@JelteF
Copy link
Owner Author

JelteF commented Jul 15, 2023

@tyranron
Copy link
Collaborator

@JelteF could you specify an example where it causes trouble? I don't understand where the conflict appears exactly.

@tyranron
Copy link
Collaborator

@JelteF ok, investigated this a liitle bit more and understood the problem. Seems like this answer does the trick even if std will ad new derives in the future.

Will commit fix shortly.

@tyranron
Copy link
Collaborator

It also makes traits visible in docs, but I do prefer this. It's much more confusing to not see traits in docs while they're imported, actually.

src/lib.rs Outdated
Comment on lines 105 to 109
// XXX: Uncommenting this causes our own derive to not be visible anymore, because the derive
// from std takes precedence somehow.
// #[cfg(feature = "debug")]
// #[doc(hidden)]
// pub use core::fmt::Debug;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, this was the example that caused issues.

@JelteF
Copy link
Owner Author

JelteF commented Jul 20, 2023

Seems like this answer does the trick even if std will ad new derives in the future.

Yeah, that's a clever trick. Using that seems fine to me.

It also makes traits visible in docs, but I do prefer this. It's much more confusing to not see traits in docs while they're imported, actually.

On one hand it's nice, on the other hand it clutters the derive_more docs quite a bit. Especially since the traits show up before the derives I think.

@tyranron
Copy link
Collaborator

@JelteF such refactoring also discovered an "iterator" Cargo feature, which simply did nothing (no actual derive existed for it).

@tyranron tyranron added this to the 1.0.0 milestone Jul 20, 2023
@tyranron tyranron marked this pull request as ready for review July 20, 2023 11:11
tyranron
tyranron previously approved these changes Jul 20, 2023
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelteF please, take a look, and if everything is OK, let's merge it!

Copy link
Owner Author

@JelteF JelteF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyranron I made some more changes. I think it's almost ready now, but I would like to remove the macros module.

src/lib.rs Outdated
Comment on lines 62 to 63
/// Use it in your import paths, if you don't want to import traits, but only macros.
pub mod macros {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot really think of a reasonable case when users would want this. I think it only clutters the docs. If people really want to do this, they could import a derive directly from derive_more_impl. So I feel we should remove this module. What do you think?

#[cfg(feature = "from_str")]
pub use self::r#str::FromStrError;
#[cfg(feature = $feature)]
#[doc(hidden)]
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really disliked how the standard library trait listing was taking up a third of the front page of the documentation. Especially beacuse they came before the documentation of the actual derives. So I hid them again and I added a small snippet to the README to say that we re-export all traits.

// all the things from that module into the main module, but we also import our own derive by its
// exact name. Due to the way wildcard imports work in rust, that results in our own derive taking
// precedence over any derive from std.
macro_rules! re_export_traits((
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a simple macro to generate the boilerplate code.

@@ -108,27 +108,12 @@ macro_rules! create_derive(
}
);

create_derive!("from", from, From, from_derive, from);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reorderd these too, to match the ordering in the other files. I think this orderering was simply the order in which the derives were added.

@JelteF JelteF enabled auto-merge (squash) July 23, 2023 13:35
@JelteF JelteF disabled auto-merge July 23, 2023 13:35
@JelteF JelteF merged commit e10b96b into master Jul 23, 2023
15 checks passed
@JelteF JelteF deleted the re-export branch July 23, 2023 13:35
@tyranron tyranron mentioned this pull request Aug 10, 2023
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.

re-export traits together with the derive
2 participants