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

Proposal: split assertContains for iterables and non-iterables #3422

Closed
keradus opened this Issue Nov 28, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@keradus
Copy link
Contributor

keradus commented Nov 28, 2018

currently, one can do both with a single assertion:

$this->assertContains('b', ['a', 'b', 'c']);
$this->assertContains('b', 'abc');

which is confusing if someone use it like the following, and for that error prone due to accidental lack of types control

$this->assertContains('b', $haystack);

what about splitting the responsibility of that assertion, to have one for strings and one for iterables ?

ref FriendsOfPHP/PHP-CS-Fixer#4061

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 28, 2018

I interpret this as

  • Add assertIterableContains() and assertStringContains() in PHPUnit 7.5
  • Deprecate (soft) assertContains() in PHPUnit 7.5
  • Deprecate (hard) assertContains() in PHPUnit 8.0

Correct?

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 28, 2018

Going further, we probably want to get rid of the optional parameters and introduce

  • assertStringContainsString() (uses new StringContains($needle, false))
  • assertStringContainsStringIgnoringCase() (uses new StringContains($needle, true))
  • assertIterableContains() (uses new TraversableContains($needle, false, false))
  • assertIterableContainsSame() (uses new TraversableContains($needle, true, true))

etc.

@sebastianbergmann sebastianbergmann self-assigned this Nov 28, 2018

@sebastianbergmann sebastianbergmann added this to the PHPUnit 7.5 milestone Nov 28, 2018

sebastianbergmann added a commit that referenced this issue Nov 28, 2018

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 28, 2018

@keradus Can you please have a look at master...686d153? Thanks!

@keradus

This comment has been minimized.

Copy link
Contributor Author

keradus commented Nov 28, 2018

Hi ! First of all, glad you like the idea !

Deprecate (soft) assertContains() in PHPUnit 7.5
Deprecate (hard) assertContains() in PHPUnit 8.0

can you elaborate how you define difference of hard and soft deprecation? if hard is removal of functionality, then that's how I would see this to happen indeed ;)

assertIterableContains() (uses new TraversableContains($needle, false, false))
assertIterableContainsSame() (uses new TraversableContains($needle, true, true))

I would rather promote strict comparison by default. Language itself is promoting to be strict about types now, let's not promote (by more convenient (/shorter) name) being not strict.
that saying, I would suggest following naming instead:
assertIterableContains to check strict
assertIterableContainsEqual to check non-strict

Can you please have a look at master...686d153? Thanks!

can you raise it as a PR? with link provided, I can't do the comments :(

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 28, 2018

  1. Soft Deprecation in PHPUnit 7.5
  2. Hard Deprecation in PHPUnit 8
  3. Removal in PHPUnit 9
@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 28, 2018

Sorry, but I do not send myself pull requests.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 28, 2018

I agree with promoting strict comparison, but the naming as-is makes more sense to me somehow.

sebastianbergmann added a commit that referenced this issue Nov 29, 2018

sebastianbergmann added a commit that referenced this issue Dec 2, 2018

More work on #3422.
* Revert deprecation of assertContains() and assertNotContains()
* Revert addition of assertIterableContains(), assertIterableContainsSame(), assertIterableNotContains(), and assertIterableNotContainsSame()
* Deprecate using assertContains() and assertNotContains() on string haystacks
* Deprecate the optional parameters $ignoreCase, $checkForObjectIdentity, and $checkForNonObjectIdentity of assertContains() and assertNotContains()

bjuppa added a commit to bjuppa/laravel-blog that referenced this issue Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.