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

Split crate into a proc-macro and a regular library crate #214

Merged
merged 11 commits into from
Nov 14, 2022

Conversation

JelteF
Copy link
Owner

@JelteF JelteF commented Oct 23, 2022

proc-macro crates are loaded by the compiler instead of by the crate
that depends on it. This means it's not possible to use anything else
than exported anything else than exported proc-macros. But in some cases
we want the generated derives to use structs or traits that are provided
by the library. Two examples of this are:

  1. An error type that can be used by the Into derive (#173)
  2. Or to support trait objects for the source of the Error derive (#122)

This change splits the layout of this crate in two. One crate that only
exports proc-macros and is loaded by the compiler. And a second crate
that re-exports all these macros but is also able to export different
things such as structs and traits.

NOTE: This only refactors the code in this repo. No functional changes
happen in this change. These will be done in follow-up PRs.

Required for #173 #122

proc-macro crates are loaded by the compiler instead of by the crate
that depends on it. This means it's not possible to use anything else
than exported anything else than exported proc-macros. But in some cases
we want the generated derives to use structs or traits that are provided
by the library. Two examples of this are:
1. An error type that can be used by the `Into` derive ([#173][173])
2. Or to support trait objects for the `source` of the `Error` derive ([#122][122])

This change splits the layout of this crate in two. One crate that only
exports proc-macros and is loaded by the compiler. And a second crate
that re-exports all these macros but is also able to export different
things such as structs and traits.

NOTE: This only refactors the code in this repo. No functional changes
happen in this change. These will be done in follow-up PRs.

[173]: #173
[122]: #122
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 I've provided my changes to the feedback as separate commits, so you may revert them granularly if you do mind about some change.

I've tried to leave you as little work as possible, considering the moments where I'm unsure how to act.

@@ -29,236 +29,5 @@
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
#![recursion_limit = "128"]
#![deny(rustdoc::broken_intra_doc_links, rustdoc::private_intra_doc_links)]
#![forbid(non_ascii_idents, unsafe_code)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the reason for these lints not being turned on in both crates simultaneously.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in ab9d379.

@@ -29,236 +29,5 @@
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
#![recursion_limit = "128"]
#![deny(rustdoc::broken_intra_doc_links, rustdoc::private_intra_doc_links)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lints too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in 2348a34.

);

create_derive!("unwrap", unwrap, Unwrap, unwrap_derive, unwrap);
pub use derive_more_impl::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

#[doc(inline)] seems to be useful here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in bbbc918.

src/lib.rs Outdated
@@ -29,236 +29,5 @@
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
#![recursion_limit = "128"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This attribute seems to belong to proc-macro crate, as required by the implementation. We don't need it for re-export.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in 30bd893.

authors = ["Jelte Fennema <github-tech@jeltef.nl>"]
license = "MIT"
repository = "https://github.com/JelteF/derive_more"
documentation = "https://docs.rs/derive_more"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intentionally not https://docs.rs/derive_more-impl?

Copy link
Owner Author

Choose a reason for hiding this comment

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

yes

impl/Cargo.toml Outdated

[dev-dependencies]
rustversion = "1.0"
trybuild = "1.0.56"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems that we don't need this in the -impl crate, as the test lay in the main crate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Meh... we still do need rustc_version due to #[cfg(nightly)] in error.md examples.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Corrected tests and their features in fd90267.

@tyranron tyranron added this to the 1.0.0 milestone Oct 26, 2022
@tyranron
Copy link
Collaborator

tyranron commented Nov 1, 2022

ping @JelteF

1 similar comment
@tyranron
Copy link
Collaborator

tyranron commented Nov 9, 2022

ping @JelteF

@JelteF JelteF enabled auto-merge (squash) November 14, 2022 08:52
@JelteF JelteF disabled auto-merge November 14, 2022 08:52
@JelteF JelteF merged commit 77e0cc8 into master Nov 14, 2022
@JelteF JelteF deleted the change-into-error branch November 14, 2022 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants