[CodeQuality] Narrow assertTrue(is_<type>()) to assertIs<Type>() - #770
Open
Soean wants to merge 2 commits into
Open
[CodeQuality] Narrow assertTrue(is_<type>()) to assertIs<Type>()#770Soean wants to merge 2 commits into
Soean wants to merge 2 commits into
Conversation
…ssertTrueFalseToSpecificMethodRector
Adds the is_<type>() family to the function map: is_array, is_bool,
is_callable, is_float, is_int, is_iterable, is_numeric, is_object,
is_scalar and is_string.
$this->assertTrue(is_callable($value)) -> $this->assertIsCallable($value)
$this->assertFalse(is_callable($value)) -> $this->assertIsNotCallable($value)
is_callable() is skipped when it carries more than one argument, as its
$syntax_only and $callable_name parameters have no counterpart in
assertIsCallable() and would end up in the $message slot.
is_resource() is left out on purpose: the IsType constraint counts
"resource (closed)" as a resource, while is_resource() returns false for
closed resources, so assertIsResource() is not an equivalent.
Also bail out on named and spread args. The rule moves the inner function
arguments up into the assert call, but the assert methods name their
parameters differently, so a named arg produces a call that cannot be
resolved:
$this->assertTrue(is_readable(filename: $file));
// became assertIsReadable(filename: $file) -> Unknown named parameter
Spread args have the same problem, as their count is unknown at compile
time. This part is pre-existing and affects the whole map, not just the
new entries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AssertTrueFalseToSpecificMethodRectoralready narrowsassertTrue(is_readable(...)),assertTrue(is_null(...))and friends, but theis_<type>()family was missing — so$this->assertTrue(is_callable($value))stayed as a generic boolean assertion instead of becoming$this->assertIsCallable($value).The generic form loses the failure message:
assertTrue()can only report "false is not true", whileassertIsCallable()reports the actual value and the type it was expected to have.Added mappings
is_arrayassertIsArrayassertIsNotArrayis_boolassertIsBoolassertIsNotBoolis_callableassertIsCallableassertIsNotCallableis_floatassertIsFloatassertIsNotFloatis_intassertIsIntassertIsNotIntis_iterableassertIsIterableassertIsNotIterableis_numericassertIsNumericassertIsNotNumericis_objectassertIsObjectassertIsNotObjectis_scalarassertIsScalarassertIsNotScalaris_stringassertIsStringassertIsNotStringEach of the ten was checked against
IsType::matches()to confirm the constraint runs the very same function, so the assertions are equivalent.is_callable()is skipped when it carries more than one argument — its$syntax_onlyand$callable_nameparameters have no counterpart inassertIsCallable()and would otherwise be moved up into the$messageslot.Also in here: named and spread args
Args are moved up by position, so a named arg only survives when the assert method happens to use the same parameter name. That holds for
is_readable(filename:),is_writable(filename:),file_exists(filename:),array_key_exists(key:, array:)andstr_contains(haystack:, needle:). It does not hold for any of the new entries, as PHP names the parameter$valuewhile PHPUnit names it$actual:The bail-out is blanket rather than per entry, which also covers
is_dir($filenamevs$directory),is_null,is_nan,is_infiniteandis_a— those carry the same mismatch today. The cost is that the handful of pairs whose names do line up get skipped too, which seemed a better trade than tracking a parameter name per mapped function. Spread args are skipped for the same reason, as their count is unknown.It sits in its own
hasUnmovableArgs()method, so it is easy to pull out if you would rather keep this PR to the new mappings.