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

Add containsStringIgnoringWhiteSpace matcher #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

manicki
Copy link
Member

@manicki manicki commented Apr 20, 2017

This would allow to make asserts against multi-line HTML pieces, see: #1.

@manicki manicki requested a review from bekh6ex April 20, 2017 13:02
Copy link
Contributor

@bekh6ex bekh6ex left a comment

Choose a reason for hiding this comment

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

I've fixed some minor stuff. Hope you don't mind.

@manicki
Copy link
Member Author

manicki commented Apr 24, 2017

I don't mind @bekh6ex, thanks!
The original commit was a bit sloppy, I admit. It was meant more of a proof-of-concept thing.
And I hope it actually it makes sense to have a matcher like this :)

@bekh6ex
Copy link
Contributor

bekh6ex commented Apr 24, 2017

And I hope it actually it makes sense to have a matcher like this :)

I guess more or less.
First I thought that we can change the behaviour of TextContentsMatcher, but then I've remembered that there are pre and code tags that care about white spaces, so I guess it is the way to go.

Copy link
Contributor

@thiemowmde thiemowmde left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, @bekh6ex.

*/
private function stripSpace( $string)
{
return trim(preg_replace( "/\s+/", ' ', $string));
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is in double quotes this technically needs to be "/\\s+/". But this is not critical. All the missing backslash does is creating a warning in IDEs.

*/
protected function evalSubstringOf($item)
{
return (false !== strpos($this->stripSpace((string) $item), $this->stripSpace($this->_substring)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it matters much, but I find this order a bit surprising.

@thiemowmde thiemowmde force-pushed the contains-string-ignoring-white-space branch from c6bdac3 to b332dcb Compare May 14, 2018 08:04
@wmde wmde deleted a comment from bekh6ex May 14, 2018
@wmde wmde deleted a comment from bekh6ex May 14, 2018
@wmde wmde deleted a comment from bekh6ex May 14, 2018
@thiemowmde
Copy link
Contributor

I rebased this patch, squashed all of the tiny commits that contained nothing but fixups for previous commits, reformatted the code according to the current PHPCS rules, and finally removed most of the now obsolete comments I wrote here in the review thread (along with some by Aleksey that said nothing but "done").

I don't know if this is still needed. Please either merge this or close #1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants