Replace lots of hamcrest's assert_that with a builder style #5929
Conversation
tests/testsuite/support/mod.rs
Outdated
| impl Drop for Execs { | ||
| fn drop(&mut self) { | ||
| if !self.ran { | ||
| // TODO: Re-enable when everything goes through Execs#run |
There was a problem hiding this comment.
Overall I like the direction. mabe add a #[must_use] to Execs?
There was a problem hiding this comment.
I'm copying Alex's implementation in rustfix: https://github.com/rust-lang-nursery/rustfix/blob/116c7e9f7b666d92c049ab86c1d83e8ced1cead4/cargo-fix/tests/all/main.rs#L323-L329
How do the two techniques compare?
There was a problem hiding this comment.
Interesting: ProjectBuilder already has the must_use attribute applied to it.
There was a problem hiding this comment.
Defence in depth, use both. One checks at runtime the other at compile time.
|
Looking good to me! I wonder if we could lift the requirement though of requiring both |
|
Yeah we can make How do you feel, instead, about running the assert in the Drop? It would remove the need to expressly execute |
|
Sounds plausible to me! (having I'd be more wary of removing the need for |
|
Looks great to me! |
dwijnand
force-pushed
the
remove-hamcrest
branch
from
47e4da6
to
85984a8
Compare
dwijnand
changed the title
|
This doesn't completely remove hamcrest, but it does a lot. I'm concerned that my grand refactorings will clash with parallel development in PRs, so I'd love to land this PR into master and send follow up PRs to close out #5742. |
|
Could someone restart AppVeyor please? |
|
Closing and reopening will trigger a retest, or pushing more commits. |
|
Yeah but both are noisy. That said my commenting is also noisy so I'm not doing any better.. |
|
Holy cow, nice! Thanks so much for taking this on! Let's definitely get this in before it bitrots! @bors: r+ |
|
📌 Commit 85984a8 has been approved by |
Replace lots of hamcrest's assert_that with a builder style Refs #5742 I've been thinking of possible solutions to close out #5742, so I'm submitting this for early feedback. @alexcrichton what do you think? If you like the look of this I'll apply it across the suite. r? @alexcrichton
|
Cheers, Alex. 🍻 |
|
☀️ Test successful - status-appveyor, status-travis |
|
And another net negative PR. If you keep up with this the project will just disappear. :-P |
Refs #5742
I've been thinking of possible solutions to close out #5742, so I'm submitting this for early feedback.
@alexcrichton what do you think? If you like the look of this I'll apply it across the suite.
r? @alexcrichton