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

Use clap instead of ::clap in clap_derive #2258

Merged
merged 1 commit into from
Dec 17, 2020
Merged

Use clap instead of ::clap in clap_derive #2258

merged 1 commit into from
Dec 17, 2020

Conversation

nitsky
Copy link
Contributor

@nitsky nitsky commented Dec 15, 2020

This PR changes clap_derive to emit code that refers to clap as just clap instead of ::clap. This will allow users who want to use a re-export of the clap crate to still use the derive macros. Below are some examples of this pattern used by some other prominent crates to allow re-exporting:

tokio: https://github.com/tokio-rs/tokio/blob/79d25b0a48d3e82a9d026e9cdfeef548153660ef/tokio-macros/src/entry.rs#L228

sqlx: https://github.com/launchbadge/sqlx/blob/04647ae09ae7f2d211c06e594561690ec9117acd/sqlx-macros/src/derives/encode.rs#L79

@pksunkara
Copy link
Member

I had done quite a bit of research on this recently for diesel derives. Removing the double colon doesn't really help with the re-exportability anyway. So, I am wondering if this is needed and all or not.

Can you describe your use case?

@nitsky
Copy link
Contributor Author

nitsky commented Dec 15, 2020

I am working in a workspace with many crates, several of which depend on clap. Rather than adding an entry in the Cargo.toml for each crate that uses clap, I would like to depend on clap just once in a crate called common which re-exports it. Downstream
crates can then use common::clap::{self, Clap}, and then #[derive(Clap)] compiles as expected.

Removing the double colon doesn't really help with the re-exportability anyway.

With this PR I am able to re-export clap from my common crate, and use the derive macros in the downstream crate. This also works for tokio and sqlx, whose derive macros use the same strategy implemented by this PR. Can you clarify what you mean by "removing the double colon doesn't really help"?

@pksunkara
Copy link
Member

If you were using use common::clap::{self, Clap} in a non root module, it wouldn't work. You would need to have a shortcut use common::clap in the root module for all targets for this workaround to work.

Which is why I meant, it would not work anyway without extra hacks.

I am inclined to merge this though. Just wanted to talk about the re-exportability issues of derive macros.

@nitsky
Copy link
Contributor Author

nitsky commented Dec 15, 2020

Is there anything else you need before merging? I see that the rustfmt check is failing, but it refers to lines that were not changed by this PR. Perhaps rustfmt needs to be run on the whole project?

If you were using use common::clap::{self, Clap} in a non root module, it wouldn't work.

Using this PR, I just tried what is shown below and it worked fine. Am I misunderstanding what you mean by "non root module"?

// lib.rs
mod file_submodule;

mod inline_submodule {
  use common::clap::{self, Clap};
  #[derive(Clap)]
  struct Args {
    pub x: String
  }
}
// file_submodule.rs
use common::clap::{self, Clap};

#[derive(Clap)]
struct Args {
  pub x: String
}

@pksunkara
Copy link
Member

Yes, please do run the rustfmt.

No, you understood right. I think it's working because the exporting lib is in the same workspace. (Yeah, I know it shouldn't matter, but I have seen this tricky thing when I was experimenting before).

@nitsky
Copy link
Contributor Author

nitsky commented Dec 15, 2020

@pksunkara I resolved the rustfmt issue, but do you know why the mingw 32-bit build failed?

@pksunkara
Copy link
Member

Pipelines in your region may be be impacted by a live site incident, resulting possible pipeline delays. Check the status here.

https://status.dev.azure.com/

@nitsky
Copy link
Contributor Author

nitsky commented Dec 17, 2020

@pksunkara Can you trigger a re-run? I do not think I have the requisite permissions.

@pksunkara pksunkara merged commit 74a5a5c into clap-rs:master Dec 17, 2020
@epage
Copy link
Member

epage commented Oct 25, 2021

Side note: structopt is instead taking the approach of allowing users to override the path. See #2921.

@pksunkara
Copy link
Member

I had explored that option for diesel too back then. It contained more friction (asking user to define the party everytime derive is used) than what the workaround is (asking to import the reexported clap in root) for the end users.

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.

None yet

3 participants