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

feat: trait_variant::make supports rewriting of the original trait. #27

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

sargarass
Copy link
Contributor

Hello, this PR should close the issue #18.

@SeaDve
Copy link

SeaDve commented Jan 27, 2024

I think this could be made more flexible by doing something like:

#[trait_variant::rewrite(Send + Sync)]
pub trait SomeTrait {
    ...
}

@tmandry
Copy link
Member

tmandry commented Jan 31, 2024

I see there's demand for this, but I too am in favor of something that more closely resembles the shape of the make macro. #[trait_variant::rewrite(Send)] is okay. Some alternatives to rewrite: add, bound

@sargarass sargarass force-pushed the main branch 2 times, most recently from a50cd80 to 3b2c682 Compare February 4, 2024 18:56
@sargarass sargarass changed the title feat: support only_send feat: support for trait_variant::rewrite Feb 4, 2024
@sargarass
Copy link
Contributor Author

I see there's demand for this, but I too am in favor of something that more closely resembles the shape of the make macro. #[trait_variant::rewrite(Send)] is okay

@SeaDve, @tmandry made the update to accomplish this

@odysa
Copy link

odysa commented Feb 5, 2024

I think it also closes my issue #29
Thank you for your work.

@tmandry
Copy link
Member

tmandry commented Feb 6, 2024

Thanks @sargarass, the implementation looks good.

As for the name, on Zulip @Jules-Bertholet suggested trait_variant::require, which I like even better than the options presented so far. What are your thoughts?

@hmacias-avaya
Copy link

would this also allow adding annotations on the trait generated by make?
For example, I need the trait generated by make to also have #[cfg_attr(test, automock)].

@sargarass
Copy link
Contributor Author

@tmandry, Hmm, as for naming, I think that trait_variant::rewrite is much closer to trait_variant::make in the sense of 'creation,' 'generation,' etc., than trait_variant::require. Because of the similarities between them, I have not suggested to name it trait_variant::with, which also sounds good but doesn't look similar.

@tmandry
Copy link
Member

tmandry commented Feb 9, 2024

The other suggestion by @traviscross was to allow #[trait_variant::make(Send)] to mean the same thing, which wouldn't require a new name. I think I'm pretty agnostic on that vs rewrite.

When I get back from vacation next week I'll work on merging one of these two, unless someone else gets to it first.

@sargarass
Copy link
Contributor Author

unless someone else gets to it first.

@tmandry well...

@traviscross
Copy link
Contributor

traviscross commented Feb 10, 2024

Over in #30 I've implemented it using the make(Send) syntax.

Given the ongoing discussion about choosing an alternate name for a different macro, I've become less ambivalent and probably would prefer to just solve this use case with make.

@sargarass sargarass changed the title feat: support for trait_variant::rewrite feat: trait_variant::make supports rewriting of the original trait. Feb 10, 2024
@sargarass
Copy link
Contributor Author

unless someone else gets to it first.

@tmandry well...

@traviscross Now we both made the same work :D

@sargarass
Copy link
Contributor Author

I've become less ambivalent and probably would prefer to just solve this use case with make.

As for the entire concept, I do support this idea

trait-variant/src/variant.rs Outdated Show resolved Hide resolved
@tmandry tmandry merged commit 91bd9eb into rust-lang:main Feb 13, 2024
1 check passed
@qrilka
Copy link

qrilka commented Feb 15, 2024

Is it planned to have this in a new release soon?
rust-lang/rust#118257 adds warnings on nightly with the current release

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.

7 participants