Page MenuHomePhabricator

add sniffs for using less specific assertions
Closed, DeclinedPublic

Description

There should be sniffs that encourage using the most specific assertion that is available.
I've created some tasks to update current uses, but for new code
T244279: assertNull should be used instead of comparing to null: assertSame( null, ... ) should be assertNull( ... )
T244552: assertTrue should be used instead of comparing to true: assertSame( true, ... ) should be assertTrue( ... )
T244553: assertFalse should be used instead of comparing to false: assertSame( false, ... ) should be assertFalse( ... )
T244554: assert(Greater|Less)Than(OrEqual)? should be used instead of assert(True|False):

  • assertTrue( a > b ) should be assertGreaterThan( b, a )
  • assertTrue( a >= b ) should be assertGreaterThanOrEqual( b, a )
  • assertTrue( a < b ) should be assertLessThan( b, a )
  • assertTrue( a <= b ) should be assertLessThanOrEqual( b, a )

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 585973 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/tools/codesniffer@master] Add a sniff for using assertNull instead of assertSame with null

https://gerrit.wikimedia.org/r/585973

Change 585973 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/tools/codesniffer@master] Add a sniff for using assertNull instead of assertSame with null

https://gerrit.wikimedia.org/r/585973

This patch now covers null, true, and false

How is any of the assertion listed in this tasks description an issue? What do we win by disallowing one form?

How is any of the assertion listed in this tasks description an issue? What do we win by disallowing one form?

I assumed that since there were no objections in 2 months that this would be fine... it makes the tests for readable (same reason I think that the assertEquals sniff replaces them with assertTrue/assertFalse/assertNull instead of assertSame)

Objections by whom? How many people have been added to this ticket? How many people have seen it?

The assertEquals sniff introduced in https://gerrit.wikimedia.org/r/547721 does not exist because of "readability". It does not touch assertSame. assertEquals is problematic because it does a relaxed == comparison internally. This results in test succeeding that most probably shouldn't.

assertSame( null, … ) and assertNull( … ) do the exact same. The only reason to replace one with the other might be that assertNull is shorter. But better readable? I think assertSame( null, … ) reads just fine.

Change 585973 abandoned by DannyS712:
Add a sniff for using assertSame with null, true, or false

https://gerrit.wikimedia.org/r/585973