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

Allow users to specify a custom path to the rust_embed crate in generated code #232

Merged
merged 1 commit into from
May 23, 2024

Conversation

Wulf
Copy link
Contributor

@Wulf Wulf commented Feb 7, 2024

Hi @pyrossh, thanks for taking the time to maintain this crate and oversee the community's contributions 🙌

I'd like to request this feature: allow end users to specify a custom path to the rust_embed crate (in case rust_embed is not in-scope, for whatever reason).

Changes

  • Added a crate_path attribute which allows the end-user to specify the path to the rust_embed crate
    #[derive(custom::path::rust_embed::RustEmbed)]
    #[crate_path = "custom::path::rust_embed"] // <--
    #[folder = "examples/public/"]
    struct Asset;
  • Added a test to make sure this doesn't break in the future :)

Notes

  • I tried to use crate as the attribute name instead of crate_path (just like serde does), but proc_macro_derive did not accept 'crate' as an attribute name.

@Wulf Wulf force-pushed the wulf/custom-crate-path branch from 113801b to f05946c Compare February 7, 2024 13:33
@pyrossh pyrossh requested a review from AzureMarker February 7, 2024 13:45
@AzureMarker
Copy link
Collaborator

I don't understand the use case. Why doesn't changing to ::rust_embed work? That's the recommended approach to the problem of when users rename a crate. See for example https://users.rust-lang.org/t/how-to-unambiguously-refer-to-a-particular-crate-from-a-proc-macro/39061. I don't think the current approach should be merged.

@Wulf
Copy link
Contributor Author

Wulf commented Feb 7, 2024

hey @AzureMarker, hope all is well. Thanks for the review :)

The use case here is to allow end-users to re-export the crate for use in another crate. This is useful in dependency management -- imagine adding rust_embed to several helper crates compared to just adding it to a single common crate that helper crates can depend on. Here's how it's currently done in serde. Let me know if I can clarify or if there's another approach you'd prefer.

@AzureMarker
Copy link
Collaborator

I see, thanks for clarifying. Personally I would use the workspace dependencies feature instead (https://doc.rust-lang.org/cargo/reference/workspaces.html#the-dependencies-table), but I understand that re-exporting has some other advantages and also works outside of a single workspace.

The changes look fine, but I'll also ask @pyrossh to approve since it's a new feature.

@pyrossh
Copy link
Owner

pyrossh commented Feb 9, 2024

@Wulf It is possible for you to try out using workspaces assuming you don't need to do much refactor? For example this is how zed have done it.

But anyways the change is pretty small and doesn't seem to break anything so can merge it. Let me know.

@Wulf Wulf closed this May 19, 2024
@Wulf Wulf force-pushed the wulf/custom-crate-path branch from f05946c to 0745567 Compare May 19, 2024 02:25
@Wulf Wulf reopened this May 19, 2024
@Wulf
Copy link
Contributor Author

Wulf commented May 19, 2024

Thanks for considering this PR still -- I've rebased on main and resolved the conflicts. I noticed cargo test --release is broken on master (edit: just checked again, this is nothing major, just some doc code failing tests).

I'd like to add that I'm not able to just use workspaces to manage deps because this feature is being used in a downstream library crate which doesn't impose project setup details on end users.

Please do merge this in @pyrossh

Comment on lines -144 to -147
fn matcher() -> ::rust_embed::utils::PathMatcher {
fn matcher() -> #crate_path::utils::PathMatcher {
#declare_includes
#declare_excludes
static PATH_MATCHER: ::std::sync::OnceLock<::rust_embed::utils::PathMatcher> = ::std::sync::OnceLock::new();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something to consider: these are the two places where we use ::rust_embed instead of rust_embed... was this for a particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a feeling this is just code style for this particular contributor based on the ::std usage

Copy link
Contributor

@BBaoVanC BBaoVanC May 19, 2024

Choose a reason for hiding this comment

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

My understanding was that you should always use the :: prefix in proc macros since it is an absolute path--it wouldn't get broken by stuff being renamed or different uses being added under the same name, outside the macro in the end user's surrounding file

Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't this also need to use #crate_path to be consistent?

@pyrossh pyrossh merged commit 7ec554e into pyrossh:master May 23, 2024
10 checks passed
@mara-schulke
Copy link

Just for documentation purposes: This PR saved us a ton of work. Thank you so much!

This attr is super useful as we want to use rust_embed within a sdk and do not want our users to add rust_embed as dependency directly, but rather use our reexport!

@mara-schulke
Copy link

@pyrossh is there a possibility on getting this released to crates.io? I believe 8.4.0 does not have this in yet unfortunately.

@pyrossh
Copy link
Owner

pyrossh commented Jul 9, 2024

@mara-schulke Released v8.5.0 with these changes.

@mara-schulke
Copy link

Thanks a lot! 🌞

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.

None yet

5 participants