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

Make Roman rule internally use Regex rule #289

Merged
merged 1 commit into from
Feb 11, 2015
Merged

Make Roman rule internally use Regex rule #289

merged 1 commit into from
Feb 11, 2015

Conversation

l-x
Copy link
Contributor

@l-x l-x commented Feb 6, 2015

Just for consistency... 😄

@henriquemoody
Copy link
Member

Since you're doing that, can you extend Regex rule instead? (like Yes rule)

@l-x
Copy link
Contributor Author

l-x commented Feb 9, 2015

I considered doing so. IMO the Roman Rule is not a special Regexp Rule, it just utilizes the Regexp Rule. Solving this by inheritance would break the OOP design principles.

@henriquemoody
Copy link
Member

The Regex rule has no specific method signature, only the same of AbstractRule; and a public variable $regex which, if needed, can be used (and useful) on Roman context as well, think about it:

function validateAndExplain(Regex $regex, $input)
{
    if (!$regex->validate($input)) {
        echo $input.' is not valid for REGEXP '.$regex->regex.PHP_EOL;
    }
}

validateAndExplain($regex, $input);
validateAndExplain($roman, $input);
validateAndExplain($yes, $input);
validateAndExplain($not, $input);

I know, Roman is not Regex, but I think better we do this than create a hard coded instance of Regex inside Roman::validate() which would break OOD principles too and, IMHO, worse them use inheritance in this case.

Unfortunately we also have other design problems and we have plans to use a different design (but keeping the fluent API) in the first major release.

@l-x
Copy link
Contributor Author

l-x commented Feb 11, 2015

Ok, I've updated the PR.

@henriquemoody henriquemoody added this to the 0.8 milestone Feb 11, 2015
@henriquemoody
Copy link
Member

Thank for contributing! 😄

henriquemoody added a commit that referenced this pull request Feb 11, 2015
Make Roman rule internally use Regex rule
@henriquemoody henriquemoody merged commit 3e46726 into Respect:master Feb 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants