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

API change? #2

Open
joshburkart opened this issue Oct 3, 2018 · 6 comments
Open

API change? #2

joshburkart opened this issue Oct 3, 2018 · 6 comments

Comments

@joshburkart
Copy link

Hi, I had put together a PR in the original hamcrest-rust project with a potential API change. Curious what you think @Valloric...?

@Valloric
Copy link
Owner

Valloric commented Oct 6, 2018

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).

@joshburkart
Copy link
Author

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...

@Valloric
Copy link
Owner

@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. :)

It would be a breaking change for users who have developed their own matchers, but not for users who just use existing ones.

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?

@joshburkart
Copy link
Author

Thanks! Sounds good. A bit busy at the moment but planning to get to this soon!

@joshburkart
Copy link
Author

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 Matcher due to GH's limited query language, but I think this is a decent indication of who might be implementing Matcher since otherwise you typically wouldn't need to reference MatchResult (right?). So that's what we're dealing with.

I think if we wanted to make the API change I'm suggesting, it would warrant a new minor version or something. Unfortunately, the README.md in the repo suggests putting in the cargo dependency as hamcrest2 = "*", so even incrementing a version would probably cause a lot of breakages. But that seems unavoidable -- we really should be able to make breaking changes sometimes.

Anyways. Thoughts?

@Valloric
Copy link
Owner

Let's do it. :)

Repository owner deleted a comment from mrpanday93 Feb 15, 2024
Repository owner deleted a comment from mrpanday93 Feb 15, 2024
Repository owner deleted a comment from mrpanday93 Feb 15, 2024
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

No branches or pull requests

2 participants