-
Notifications
You must be signed in to change notification settings - Fork 8
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
API change? #2
Comments
Thanks for your interest in contributing! :) As you've noticed, upstream hamcrest-rust seems dead; I'm open to PRs. As a general rule, I do not break backwards compatibility unless absolutely necessary. Any API changes need to not break users. If you can tweak your changes to make them compatible, open a new PR on this repo with your proposed changes and I'll look at it more closely (I haven't give a detailed look). |
It would be a breaking change for users who have developed their own matchers, but not for users who just use existing ones. The PR description explains the change in detail, so it would probably be best if you took a look at that first... |
@joshburkart Looked at the PR. I think your API is quite nice actually. Thanks for sending that out in the first place! The more people out there thinking about better testing APIs, the better. :)
That might be good enough. Creating custom matchers is rare, but it would be good to collect data on this. A possible way this could be settled for good: search through Github for code creating custom hamcrest-rust Matchers. If there's only a handful of instances, I'm sold. Even if it's more than a handful, it might still be OK. What do you think? |
Thanks! Sounds good. A bit busy at the moment but planning to get to this soon! |
Good call @Valloric -- I searched through GH for "hamcrest" and "MatchResult" (restricted to Rust code), and turned up 29 results. It's hard to directly search for code implementing I think if we wanted to make the API change I'm suggesting, it would warrant a new minor version or something. Unfortunately, the Anyways. Thoughts? |
Let's do it. :) |
Hi, I had put together a PR in the original hamcrest-rust project with a potential API change. Curious what you think @Valloric...?
The text was updated successfully, but these errors were encountered: