-
Notifications
You must be signed in to change notification settings - Fork 61
Initial basic implementation #1
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
Conversation
src/Comparator.php
Outdated
|
||
foreach ($oldApi->getAllClasses() as $oldClass) { | ||
try { | ||
$newClass = $newApi->reflect($oldClass->getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach only looks for BC breaks, not API additions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand, this one is to look for public API removal, see catch
.
Jeez man, gimme a break, I literally just started this :P |
src/Comparator.php
Outdated
continue; | ||
} | ||
|
||
foreach ($oldClass->getMethods() as $oldMethod) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be isolated into a private method
src/Comparator.php
Outdated
continue; | ||
} | ||
|
||
foreach ($oldMethod->getParameters() as $oldParameter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And again
src/Comparator.php
Outdated
} | ||
|
||
foreach ($oldMethod->getParameters() as $oldParameter) { | ||
$newParameter = $newMethod->getParameter($oldParameter->getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter should not be fetched by name, but by position. A simple test of a BC compliant change is:
before:
function foo(int $a, string $b) {}
after:
function foo(int $b, string $a) {}
Also, a rename per-se does not cause a BC break:
before:
function foo(int $a, string $b) {}
after:
function foo(int $a, string $c) {}
4475b2c
to
f6d4790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea. I think supporting http://symfony.com/doc/current/contributing/code/bc.html would be really good.
@samdark thanks for your comments; it's still a little way off yet, hopefully I'll find time soon to work on this again :) |
|
||
final class Comparator | ||
{ | ||
public function compare(ClassReflector $oldApi, ClassReflector $newApi): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we only compare classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return type array
is too limited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now yes, if you want to support functions, add an issue :P
return $changelog; | ||
} | ||
|
||
private function examineClass(array $changelog, ReflectionClass $oldClass, ClassReflector $newApi): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid returning array
?
|
||
final class Comparator | ||
{ | ||
public function compare(ClassReflector $oldApi, ClassReflector $newApi): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return type array
is too limited
ReflectionClass $oldClass, | ||
ReflectionMethod $oldMethod, | ||
ReflectionClass $newClass | ||
): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: array
is too limited
Created #13 to discuss the actual API to be used further 👍 for now merging so other functionality can start to be added |
No description provided.