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

A "LIKE" specification #29

Merged
merged 3 commits into from
Aug 29, 2014
Merged

A "LIKE" specification #29

merged 3 commits into from
Aug 29, 2014

Conversation

AP-Hunt
Copy link
Contributor

@AP-Hunt AP-Hunt commented Aug 28, 2014

Nyholm, cakper

I really like your library. I've evaluated it for use at work, and it seems really easy to use and extend. Whilst using it, though, I found a couple of little things I thought were missing.

This pull request is for the first of the missing things I identified: a "like" specification. It's pretty simple and builds on your existing work with other types of comparison. I'm not sure how I feel about taking a pattern as the format and calling sprintf($format, $value), but I think it works for a first iteration.

I'm also not sure about the order of the parameters. I think it'd be more readable to take the format between field and value parameters (ie new S\Like("baz", S\Like::CONTAINS, "bing")), but that wouldn't match the signature of the other specifications and wouldn't allow a default parameter. What do you think?

Usage example:

use Happy\Doctrine\Specification\Spec as S;
// ...
$spec new S\AndX(
    new S\Equals("foo", "bar"),
    new S\Like("baz", "bing", S\Like::STARTS_WITH)
)

Regards,
Andy

P.S. Do the spec tests look OK? I've not used PHPSpec before.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.04%) when pulling fccca36 on AndyBursh:master into 35e4b22 on Happyr:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) when pulling 668ce73 on AndyBursh:master into 35e4b22 on Happyr:master.

@AP-Hunt
Copy link
Contributor Author

AP-Hunt commented Aug 28, 2014

My apologies, I pushed an extra commit (668ce73) here for something else. I didn't expect it to also add it to this pull request.

@Nyholm
Copy link
Member

Nyholm commented Aug 28, 2014

Thank you @AndyBursh for your PR.
I like the order of the parameters that you've submitted. I know that new S\Like("baz", S\Like::CONTAINS, "bing") is easier to read but good coding standards say that optional parameters should be at the end. So keep it the way you have it now.

I think the PHPSpec looks good. @cakper is really the expert there =)

I would like you to add a factory function for the Like spec. Just ad a function in this class.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.79%) when pulling a040cf5 on AndyBursh:master into 35e4b22 on Happyr:master.

@AP-Hunt
Copy link
Contributor Author

AP-Hunt commented Aug 28, 2014

Of course, not a problem. The factory method is implemented in a040cf5.

@cakper
Copy link
Contributor

cakper commented Aug 29, 2014

👍 thanks for your work :) specs are good :) nothing to add there, I'm happy that you managed to write them even tho you never used PhpSpec

@Nyholm I'm fine with this PR, mergning

cakper added a commit that referenced this pull request Aug 29, 2014
@cakper cakper merged commit 8b0c0ee into Happyr:master Aug 29, 2014
@Nyholm
Copy link
Member

Nyholm commented Aug 29, 2014

👍

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