Case insensitive method names #99

emmetog · 2018-02-12T08:29:36Z

Description:

This is a resurrection of #96 with the same outcome but a different implementation. In #96 we added logic to the ClassMethodAnalyzer, in this PR things are a lot cleaner.

The end goal is to trigger case-changes in method names (which PHP ignores) as a PATCH change.

Features:

Lots of tests
Added .editorconfig file to ensure coding standards are consistent across library (use tabs)

Still to do:

Allow case changes in Class names
Allow case changes in Interface names
Allow case changes in Trait names
Allow case changes in Method & Function names

emmetog · 2018-02-12T08:32:36Z

src/PHPSemVerChecker/Configuration/LevelMapping.php

@@ -121,6 +121,9 @@ class LevelMapping
 		'V115' => Level::MAJOR,
 		'V116' => Level::MAJOR,
 		'V117' => Level::MAJOR,
+		'V150' => Level::PATCH,
+		'V151' => Level::PATCH,
+		'V152' => Level::PATCH,


I used the extract-rules-from-ruleset.php script to generate this entire list.

emmetog · 2018-02-12T08:35:06Z

@tomzx am I on the right track with this PR?

I haven't yet checked to see if this works for traits, interfaces and function names, I'll do this soon.

emmetog · 2018-02-14T07:54:51Z

src/PHPSemVerChecker/Operation/ClassRenamedCaseOnly.php

+use PhpParser\Node\Stmt\Class_;
+use PHPSemVerChecker\Node\Statement\Class_ as PClass;
+
+class ClassRenamedCaseOnly extends Operation {


@tomzx should this extend from a new class called ClassOperationDelta or ClassOperationUnary or something similar?

Right now the constructor for ClassRenamedCaseOnly is public function __construct($fileAfter, Class_ $classAfter), is this correct?

Yes, all of the operations should extend either the unary of delta variant. But this means it would require the creation of these classes for class, trait, interface as well. I guess this can be done later on in a refactoring pass.

I've just pushed @ 7944fd1 which creates said classes.

tomzx

I think rename shouldn't be used as the name stays the same, only the casing changes. I'm saying that because it might happen in the future that we want to track name change (for example the typing stays the same but the variable changes name). Maybe something like Class/Function/Interface/Trait name case changed (to better follow the added/changed/removed nomenclature).

tomzx · 2018-02-15T18:42:19Z

tests/PHPSemVerChecker/Analyzer/ClassMethodAnalyzerTest.php

-			['trait', 'private', 'V054'],
-		];
-	}
+    public function providerImplementationChanged()


It seems all this code is using space indentation.

tomzx · 2018-02-15T18:55:07Z

docs/Ruleset.md

@@ -158,6 +162,8 @@ V114 | MAJOR | Trait private method parameter default removed
 V115 | MAJOR | Trait public method parameter default value changed
 V116 | MAJOR | Trait protected method parameter default value changed
 V117 | MAJOR | Trait private method parameter default value changed
+V152 | PATCH | Trait method renamed (case only)
+V155 | PATCH | Trait renamed (case only)

 # To classify


It is missing function renaming.

tomzx · 2018-02-15T19:02:11Z

tests/PHPSemVerChecker/Analyzer/ClassMethodAnalyzerTest.php

+        ];
+    }
+
+    public function testClassMethodCaseChangeIsIgnored()


This needs to be tested against classes, traits and interfaces, at each visibility (public, protected, private). Use a provider similar to providerImplementationChanged to accomplish this.

The method name shouldn't be testClassMethodCaseChangeIsIgnored but testClassMethodNameCaseChanged.

tomzx · 2018-02-15T19:03:12Z

docs/Ruleset.md

@@ -70,6 +70,8 @@ V096 | PATCH | Class private method parameter default removed
 V097 | MAJOR | Class public method parameter default value changed
 V098 | MAJOR | Class protected method parameter default value changed
 V099 | PATCH | Class private method parameter default value changed
+V150 | PATCH | Class public/protected/private method renamed (case only)


This should be split into 3 rules, one for each visibility level (as it is done for all other rules even if they are all the same value).

tomzx · 2018-02-15T19:03:19Z

docs/Ruleset.md

@@ -158,6 +162,8 @@ V114 | MAJOR | Trait private method parameter default removed
 V115 | MAJOR | Trait public method parameter default value changed
 V116 | MAJOR | Trait protected method parameter default value changed
 V117 | MAJOR | Trait private method parameter default value changed
+V152 | PATCH | Trait method renamed (case only)


This should be split into 3 rules, one for each visibility level (as it is done for all other rules even if they are all the same value).

tomzx · 2018-02-15T19:03:22Z

docs/Ruleset.md

@@ -109,6 +111,8 @@ V076 | MAJOR | Interface method parameter typing removed
 V077 | MINOR | Interface method parameter default added
 V078 | MAJOR | Interface method parameter default removed
 V079 | MAJOR | Interface method parameter default value changed
+V151 | PATCH | Interface method renamed (case only)


This should be split into 3 rules, one for each visibility level (as it is done for all other rules even if they are all the same value).

Interfaces can only have public methods

True. In this case it should only be for the public method visibility. (the message was copy/pasted in each case 😉)

tomzx

Looks good overall.

Other small general comment, the style is that there's a space between the keyword (if/foreach/for/while/etc.) and the opening parenthesis, and a space between the closing parenthesis and opening brace, such as keyword (...) { ... }

tomzx · 2018-02-15T19:14:31Z

src/PHPSemVerChecker/Operation/TraitRenamedCaseOnly.php

+use PhpParser\Node\Stmt\Trait_;
+use PHPSemVerChecker\Node\Statement\Trait_ as PTrait;
+
+class TraitRenamedCaseOnly extends Operation {


Should be a delta operation.

tomzx · 2018-02-15T19:14:53Z

src/PHPSemVerChecker/Operation/InterfaceRenamedCaseOnly.php

+use PhpParser\Node\Stmt\Interface_;
+use PHPSemVerChecker\Node\Statement\Interface_ as PInterface;
+
+class InterfaceRenamedCaseOnly extends Operation {


Should be a delta operation.

tomzx · 2018-02-15T19:15:04Z

src/PHPSemVerChecker/Operation/ClassRenamedCaseOnly.php

+use PhpParser\Node\Stmt\Class_;
+use PHPSemVerChecker\Node\Statement\Class_ as PClass;
+
+class ClassRenamedCaseOnly extends Operation {


Should be a delta operation.

tomzx · 2018-02-15T19:16:50Z

src/PHPSemVerChecker/Analyzer/ClassAnalyzer.php

+		$classesAfter = $registryAfter->data['class'];
+
+		$classesBeforeKeyed = [];
+		$mappingsBeforeKeyed = [];


Maybe prefer filesBeforeKeyed?

tomzx · 2018-02-15T19:17:00Z

src/PHPSemVerChecker/Analyzer/ClassAnalyzer.php

+		}
+
+		$classesAfterKeyed = [];
+		$mappingsAfterKeyed = [];


Maybe prefer filesAfterKeyed?

tomzx · 2018-02-15T19:22:10Z

tests/PHPSemVerChecker/Analyzer/ClassMethodAnalyzerTest.php

+        ];
+    }
+
+    public function testClassMethodCaseChangeIsIgnored()


The method name shouldn't be testClassMethodCaseChangeIsIgnored but testClassMethodNameCaseChanged.

tomzx · 2018-02-15T19:22:44Z

tests/PHPSemVerChecker/Analyzer/TraitAnalyzerTest.php

@@ -62,4 +62,23 @@ public function testTraitAdded()
 		$this->assertSame('Trait was added.', $report[$context][$expectedLevel][0]->getReason());
 		$this->assertSame('tmp', $report[$context][$expectedLevel][0]->getTarget());
 	}
+
+	public function testTraitRenamedCaseOnly()


The method name should be testTraitNameCaseChanged.

tomzx · 2018-02-15T19:23:22Z

tests/PHPSemVerChecker/Analyzer/InterfaceAnalyzerTest.php

@@ -62,4 +62,25 @@ public function testInterfaceAdded()
 		$this->assertSame('Interface was added.', $report[$context][$expectedLevel][0]->getReason());
 		$this->assertSame('tmp', $report[$context][$expectedLevel][0]->getTarget());
 	}
+
+	public function testInterfaceRenamedCaseOnly()


The method name should be testInterfaceNameCaseChanged.

tomzx · 2018-02-15T19:27:53Z

src/PHPSemVerChecker/Analyzer/ClassMethodAnalyzer.php


-				$signatureResult = Signature::analyze($paramsBefore, $paramsAfter);
+				// Detect method renamed case only.
+				if(


Break long comparison into variables and test against those, e.g.:

$isSameName = strtolower($methodBefore->name) === strtolower($methodAfter->name); $isExactName = $methodBefore->name === $methodAfter->name; if ($isSameName && !$isExactName) { ... }

Any reason you need to check strtolower($methodBefore->name) === strtolower($methodAfter->name)? They're technically mapped to be since they're part of the $methodsToVerify array.

emmetog · 2018-02-15T21:50:04Z

A huge thanks for your feedback @tomzx. I'll work on fixing these up over the next few days.

emmetog · 2018-02-19T07:56:09Z

@tomzx I've cleaned up those few things ;)

tomzx · 2018-02-22T20:21:23Z

src/PHPSemVerChecker/Analyzer/FunctionAnalyzer.php


-				$signatureResult = Signature::analyze($paramsBefore, $paramsAfter);
+				// Check if the name of the interface has changed case.


It should say "of the function", not "of the interface".

Also make sure not to have a space between the if and the comment.

tomzx · 2018-02-22T20:25:54Z

tests/PHPSemVerChecker/Analyzer/ClassMethodAnalyzerTest.php

+	/**
+	 * @dataProvider providerCaseChanged
+	 */
+	public function testClassMethodCaseChangeChanged($context, $visibility, $code)


The method name shouldn't be testClassMethodCaseChangeChanged but testClassMethodCaseChanged.

tomzx · 2018-02-22T20:33:35Z

Looks good enough now. I'll merge and make a few final fixes.

emmetog added 6 commits February 8, 2018 08:18

Dont treat case changes in method names as breaking

e6cb7e2

Change indentation to tabs

4f565b6

Add some tests for Signature comparator

a382291

Detect method name case changes

63f11e9

Make renaming method case a PATCH change

d2418c3

Remove commented var_dump

6424295

emmetog commented Feb 12, 2018

View reviewed changes

emmetog added 5 commits February 13, 2018 08:51

Detect interface case name changes as PATCH

5b091f4

Revert Signature::analyze

d0191d8

Detect case only name change for Traits

b9de798

Detect case only name changes for Classes

558aaae

Add rule for trait renamed case only

f8e54b3

emmetog commented Feb 14, 2018

View reviewed changes

tomzx reviewed Feb 15, 2018

View reviewed changes

emmetog added 13 commits February 19, 2018 06:23

Split method and trait rename into visibility

e1d49be

Add tests for all visibilities of case change

9ca9220

Merge branch 'master' into case-insensitive-method-names

50b291d

Interface operation extend from Delta

e527747

Rename $mappingsBeforeKeyed to $filesBeforeKeyed

596646c

Remove unneccessary compare of lowercase names

0c24711

Remove empty line

d49730c

Make ClassMethodRenamedCaseOnly extend Delta

a15c6b7

Make TraitRenamedCaseOnly extend Delta

fbdb940

Spaces to tabs

d672b2d

Rename test methods

ce3e042

Detect function renaming too

667e0be

Rename all operations to CaseChanged

e50e738

Use case changed instead of rename in rule names

931d81f

tomzx reviewed Feb 22, 2018

View reviewed changes

tomzx merged commit 931d81f into tomzx:master Feb 22, 2018

tomzx mentioned this pull request Feb 24, 2018

Mark classes, classes methods, functions, traits and interfaces as PATCH when their casing is changed #98

Closed

tomzx added this to the v0.13.0 milestone Feb 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Case insensitive method names #99

Case insensitive method names #99

emmetog commented Feb 12, 2018 •

edited

Loading

emmetog Feb 12, 2018

emmetog commented Feb 12, 2018

emmetog Feb 14, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx left a comment

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

emmetog Feb 19, 2018

tomzx Feb 19, 2018

tomzx left a comment

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

tomzx Feb 15, 2018

emmetog commented Feb 15, 2018

emmetog commented Feb 19, 2018

tomzx Feb 22, 2018 •

edited

Loading

tomzx Feb 22, 2018 •

edited

Loading

tomzx commented Feb 22, 2018 •

edited

Loading


		$signatureResult = Signature::analyze($paramsBefore, $paramsAfter);
		// Check if the name of the interface has changed case.

Case insensitive method names #99

Case insensitive method names #99

Conversation

emmetog commented Feb 12, 2018 • edited Loading

Description:

Features:

Still to do:

Choose a reason for hiding this comment

emmetog commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomzx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomzx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmetog commented Feb 15, 2018

emmetog commented Feb 19, 2018

tomzx Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

tomzx Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

tomzx commented Feb 22, 2018 • edited Loading

emmetog commented Feb 12, 2018 •

edited

Loading

tomzx Feb 22, 2018 •

edited

Loading

tomzx Feb 22, 2018 •

edited

Loading

tomzx commented Feb 22, 2018 •

edited

Loading