Initial support for trait-variant#640
Initial support for trait-variant#640asomers merged 6 commits intoasomers:masterfrom lasantosr:trait-variant-support
Conversation
|
@asomers I think the nightly is failing because some sort of new lints unrelated to this PR, would you like me to fix those also here? |
It's already fixed on master. You just need to rebase. |
|
Great! It looks like it's ready now |
asomers
left a comment
There was a problem hiding this comment.
I think the tests could really benefit from some kind of assertion to check that they actually implement the additional trait. That might be hard to do with Send, but what about something like this?
trait Foo {}
#[automock]
#[trait_variant::make(Foo)]
pub trait Bar {
...
}
#[test]
fn some_test_method_name() {
let mock = MockBar::new();
let p: Box<dyn Foo> = Box::new(mock); // Ensures that MockBar implements Foo
...
}Also, with this change, does the #[trait_variant::make(OtherTraitName: Foo)] syntax work?
I'm not that familiar with the codebase to implement all of those changes. On your first example, automock would not implement trait Foo {}
#[automock]
#[trait_variant::make(Foo)]
pub trait Bar {
...
}
impl Foo for MockBar {}The most common use case is to mark your trait (and async method futures) As for the second example, the syntax already works, but we would be mocking the initial trait only ( The main goal of this PR is to have feature parity with async_trait, other improvements could be added later. As mentioend on #639 the async_trait annotation can be located before or after the automock annotation, and that means you need to return the actual pinned future or just the future's output on the mocks expectations. With this PR we can do the same for trait_variant, so that the migration is painless. |
|
@asomers can this PR be merged as is to allow initial basic support for trait_variant? Other improvements can be added later, when needed |
asomers
left a comment
There was a problem hiding this comment.
We need to decide what do do about the #[trait_variant::make(IntFactory: Send)] syntax. It might just work. Have you tried it? If so, we should add tests for it. If not, and it won't be easy to do, then you need to at least document that.
Also, there must be a CHANGELOG entry.
@asomers It was already working but it mocks the original trait (not the variant) On this example, #[automock]
#[trait_variant::make(Foo: Send)]
pub trait LocalFoo {
async fn foo(&self) -> u32;
async fn bar() -> u32;
}I've extended automock to allow mocking the variant as well (I've included examples and updated automock documentation) On this example, #[automock(target = Foo)]
#[trait_variant::make(Foo: Send)]
pub trait LocalFoo {
async fn foo(&self) -> u32;
async fn bar() -> u32;
}Note: All of the other comments have been resolved, except for the static assertion which is not required. If you still want me to include it let me know. |
asomers
left a comment
There was a problem hiding this comment.
This is looking pretty good now. But I do want to use static_assertions, like I mentioned.
| self.doc | ||
| } else if *i.as_ref().unwrap() == "async_trait" { | ||
| self.async_trait | ||
| } else if *i.as_ref().unwrap() == "make" { |
There was a problem hiding this comment.
It's unfortunate that the most significant word in this attribute name isn't the last one "make", but the first "trait_variant". That's unlike every other attribute that we process. And we may have to alter our processing in the future, if another crate defines an attribute called "make". But for now we can leave it.
|
@asomers It should be ready now! Thanks for the review |
asomers
left a comment
There was a problem hiding this comment.
That looks much better. I have just one more request: could you please add a CHANGELOG entry?
|
@asomers There's already a changelog entry, let me know if you would like another sentence. |
Of course you did. But yesterday I was only viewing "changes since your last review" so I didn't see it. My bad. |
This PR aims to fix #639
Feel free to update the readme, as English is not my primary language