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

WIP: Potential API change #52

Closed
wants to merge 1 commit into from
Closed

Conversation

joshburkart
Copy link

Hi!

I was trying to play around with this awesome library, and in so doing came up with some (potentially not completely backwards-compatible...) API changes that I think might be worthwhile. Thought I'd throw them together into a PR to get some feedback (prior to updating all the extant Matcher impls to adhere to the new API).

The main thing is to ask Matchers to return the following complete set of information in a structured way:

  1. What the Matcher looks like.
  2. What the Matcher thinks the "actual" value should look like.
  3. Whether the match was successful or not.
    1. And optionally an explanation for why the match was unsuccessful.

I also put in a slightly modified template for match-failure panic messages:

  Expected: <formatted matcher>
       Got: <formatted actual value>

Or, if an optional explanation is attached:

  Expected: <formatted matcher>
       Got: <formatted actual value> (<explanation>)

DO NOT MERGE. At the moment this is just a vessel for feedback.

@povilasb
Copy link
Contributor

Looks cool 👍
Also, you can prefix PR name with "WIP: " to explicitly state that it's not supposed to be merged. E.g.
WIP: Potential API change

@joshburkart joshburkart changed the title Potential API change WIP: Potential API change Mar 20, 2018
@joshburkart
Copy link
Author

So I guess the question is: should I make a real PR with updated default matchers, etc.?

@povilasb
Copy link
Contributor

Ping @ujh

@joshburkart
Copy link
Author

@ujh seems to be MIA? @povilasb What's the path forward?

@povilasb
Copy link
Contributor

Hmmm, the only way I see is forking. But I really don't wanna do that. In that case we'd still need @ujh to help publish new versions on crates.io or transfer permissions etc.
I'd really like to get some feedback from @ujh first.

@povilasb
Copy link
Contributor

povilasb commented Oct 3, 2018

@joshburkart you should take a look at https://github.com/Valloric/hamcrest2-rust ;)

@joshburkart joshburkart closed this by deleting the head repository Jun 9, 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

Successfully merging this pull request may close these issues.

None yet

2 participants