-
Notifications
You must be signed in to change notification settings - Fork 61
Run api-compare via CLI #15
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
Note: some tests are missing, and |
/** @var Change[] */ | ||
private $changes = []; | ||
|
||
private function __construct() |
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.
What is the purpose of a private constructor in this case?
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.
Named constructors etc.
{ | ||
} | ||
|
||
public static function new(): self |
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.
Reinventing the wheel i see
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.
I agree. It does not capture any meaningful value here.
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.
There are two ways to construct this:
::new()
::fromArray()
Fully qualifies having static named constructors here to avoid potential BC breaks in the future
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.
Hmm, That is a valid point and I guess it also follows the "conventions" of the project.
|
||
final class ApiCompare extends Command | ||
{ | ||
/** @var PerformCheckoutOfRevision */ |
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.
Please expand like the property below
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.
Oh, yeh the CS will eventually be this (non-expanded)
|
||
final class Changes implements IteratorAggregate | ||
{ | ||
/** @var Change[] */ |
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.
Please expand
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.
eh?
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.
nm, see above
{ | ||
/** @var Change $change */ | ||
foreach ($changes as $change) { | ||
$this->output->writeln((string)$change); |
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.
Add some color, be hipster.
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.
Nice idea; I'll add an issue but not bothering here
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.
See #16
->with(sprintf('[BC] REMOVED: %s', $change1Text)); | ||
$output->expects(self::at(1)) | ||
->method('writeln') | ||
->with(sprintf(' ADDED: %s', $change2Text)); |
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.
Why the extra
space?
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 line up with [BC]
src/Command/ApiCompare.php
Outdated
$fromPath = $this->git->checkout($sourceRepo, Revision::fromSha1($input->getArgument('from'))); | ||
$toPath = $this->git->checkout($sourceRepo, Revision::fromSha1($input->getArgument('to'))); | ||
|
||
// @todo fix hard-coded /src/ addition... |
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.
Detect composer-json or psr4 loading?
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.
Nice idea - added #17
Tests added and passing; no longer WIP and ready for review & merge |
… into feature/provide-object-api-for-changes
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.
Prototype here for running via CLI.
Usage currently like:
Note: this is a VERY rough around the edges initial implementation, and doesn't satisfy all our requirements yet (from #13). For example it doesn't do revision parsing (so it has to be full revision at the moment).