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

README: add precision on custom matchers [ci skip] #125

Closed
wants to merge 8 commits into from

Conversation

arnlen
Copy link
Contributor

@arnlen arnlen commented Apr 25, 2017

Following #124, a few more explanations on custom matchers.

Copy link
Collaborator

@MatyasKriz MatyasKriz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestions for fixing some of the grammatical errors as well as some stylistic tips.

README.md Outdated

#### B) Custom matchers

If Cuckoo doesn't know to type your try to compare, you have to write your own method `equal(to:)` using a `ParameterMatcher`. Add this this method to your test file:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be "If Cuckoo doesn't know the type you are trying to compare, ..."

Also, there is duplicate "this" in the next sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 👍

README.md Outdated

For details and implementation example (with Alamofire), see [this issue](https://github.com/Brightify/Cuckoo/issues/124).

#### Parameter matchers

`ParameterMatcher` also conform to `Matchable`. You can create your own `ParameterMatcher` instances or if you want to directly use your custom types there is the `Matchable` protocol. Standard instances of `ParameterMatcher` can be obtain via these functions:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"also conforms to ..."

In the last sentence: "can be obtained ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 👍

README.md Outdated
}
```

⚠️ If you try to match an object with an unknown or non-`Matchable` type, this could lead to a:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be subjective, but I feel that "this could lead to a:" would feel more natural as "it can lead to:". Removing the "a" as well, I would suggest keep it if it was one sentence, but when you're using a colon, it seems pointless.

My reasoning is that "this" in this sense is mainly used at the beginning of sentences, but here it's not a new sentence.
Consider these two shorter examples,
"If you fall down, it can lead to a hospital visit."
and
"If you fall down, this can lead to a hospital visit."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clear and instructive: thanks a lot for those explanations! 👍 It makes totally sense, and I've updated the doc accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to help. :)

@arnlen
Copy link
Contributor Author

arnlen commented Apr 27, 2017

Thx a lot @sparkesix for this review! I'll make the suggested changes. 👍

@MatyasKriz
Copy link
Collaborator

Merged. Thanks for the PR!

@MatyasKriz MatyasKriz closed this Mar 23, 2018
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.

4 participants