From 348ec7da60a6e19d5a853555f1a05df4d58017dc Mon Sep 17 00:00:00 2001 From: Peter Dohogne Date: Wed, 26 Feb 2020 14:00:39 -0600 Subject: [PATCH] Code review changes --- src/Analyzer/AbstractCodeAnalyzer.php | 2 +- src/Analyzer/ClassAnalyzer.php | 4 +- src/Analyzer/InterfaceAnalyzer.php | 5 +- src/Analyzer/TraitAnalyzer.php | 6 +- src/DbSchemaReport.php | 22 ++--- src/MergedReport.php | 33 +++++++ src/ReportBuilder.php | 12 +-- src/Reporter/BreakingChangeTableReporter.php | 8 +- tests/Unit/MergedReportTest.php | 96 ++++++++++++++++++++ 9 files changed, 149 insertions(+), 39 deletions(-) create mode 100644 src/MergedReport.php create mode 100644 tests/Unit/MergedReportTest.php diff --git a/src/Analyzer/AbstractCodeAnalyzer.php b/src/Analyzer/AbstractCodeAnalyzer.php index e9f92377..1bf4d16a 100644 --- a/src/Analyzer/AbstractCodeAnalyzer.php +++ b/src/Analyzer/AbstractCodeAnalyzer.php @@ -244,7 +244,7 @@ protected function getFileName($context, $nodeName, $isBefore = true) * @param string $context * @param string $fileBefore * @param string $fileAfter - * @return array + * @return AbstractCodeAnalyzer[] */ protected function getContentAnalyzers($context, $fileBefore, $fileAfter) { diff --git a/src/Analyzer/ClassAnalyzer.php b/src/Analyzer/ClassAnalyzer.php index 3ce236c6..51b4b8a2 100644 --- a/src/Analyzer/ClassAnalyzer.php +++ b/src/Analyzer/ClassAnalyzer.php @@ -108,7 +108,6 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe $classAfter = $afterNameMap[$key]; if ($classBefore !== $classAfter) { - /** @var AnalyzerInterface[] $analyzers */ $analyzers = $this->getContentAnalyzers(static::CONTEXT, $fileBefore, $fileAfter); foreach ($analyzers as $analyzer) { @@ -125,7 +124,7 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe * @param string $context * @param string $fileBefore * @param string $fileAfter - * @return array + * @return AbstractCodeAnalyzer[] */ protected function getContentAnalyzers($context, $fileBefore, $fileAfter) { @@ -140,7 +139,6 @@ protected function getContentAnalyzers($context, $fileBefore, $fileAfter) ]; } - /** * Get the class node registry * diff --git a/src/Analyzer/InterfaceAnalyzer.php b/src/Analyzer/InterfaceAnalyzer.php index 4678a3ec..165ef39f 100644 --- a/src/Analyzer/InterfaceAnalyzer.php +++ b/src/Analyzer/InterfaceAnalyzer.php @@ -118,7 +118,6 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe $interfaceAfter = $afterNameMap[$key]; if ($interfaceBefore !== $interfaceAfter) { - /** @var AnalyzerInterface[] $analyzers */ $analyzers = $this->getContentAnalyzers(static::CONTEXT, $fileBefore, $fileAfter); foreach ($analyzers as $analyzer) { @@ -135,7 +134,7 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe * @param string $context * @param string $fileBefore * @param string $fileAfter - * @return array + * @return AbstractCodeAnalyzer[] */ protected function getContentAnalyzers($context, $fileBefore, $fileAfter) { @@ -146,6 +145,4 @@ protected function getContentAnalyzers($context, $fileBefore, $fileAfter) new InterfaceExtendsAnalyzer($context, $fileBefore, $fileAfter) ]; } - - } diff --git a/src/Analyzer/TraitAnalyzer.php b/src/Analyzer/TraitAnalyzer.php index 9e7d5303..084996a7 100644 --- a/src/Analyzer/TraitAnalyzer.php +++ b/src/Analyzer/TraitAnalyzer.php @@ -115,8 +115,6 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe if ($traitBefore !== $traitAfter) { $fileBefore = $this->getFileName($registryBefore, $traitName); $fileAfter = $this->getFileName($registryAfter, $traitName); - - /** @var AbstractCodeAnalyzer[] $analyzers */ $analyzers = $this->getContentAnalyzers(static::CONTEXT, $fileBefore, $fileAfter); foreach ($analyzers as $analyzer) { @@ -133,12 +131,10 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe * @param string $context * @param string $fileBefore * @param string $fileAfter - * @return array + * @return AbstractCodeAnalyzer[] */ protected function getContentAnalyzers($context, $fileBefore, $fileAfter) { return [new ClassMethodAnalyzer($context, $fileBefore, $fileAfter)]; } - - } diff --git a/src/DbSchemaReport.php b/src/DbSchemaReport.php index 5924a6d9..e5a17607 100644 --- a/src/DbSchemaReport.php +++ b/src/DbSchemaReport.php @@ -11,19 +11,6 @@ class DbSchemaReport extends ReportAlias { - public static $contexts = [ - 'class', - 'function', - 'interface', - 'trait', - 'database', - 'di', - 'layout', - 'system', - 'xsd', - 'less' - ]; - /** * Report constructor. */ @@ -31,8 +18,11 @@ public function __construct() { $levels = array_fill_keys(Level::asList(), []); parent::__construct(); - foreach (static::$contexts as $context) { - $this->differences[$context] = $levels; - } + $this->differences['database'] = $levels; + $this->differences['di'] = $levels; + $this->differences['layout'] = $levels; + $this->differences['system'] = $levels; + $this->differences['xsd'] = $levels; + $this->differences['less'] = $levels; } } diff --git a/src/MergedReport.php b/src/MergedReport.php new file mode 100644 index 00000000..d8e76bef --- /dev/null +++ b/src/MergedReport.php @@ -0,0 +1,33 @@ +differences as $context => $levels) { + if (!key_exists($context, $this->differences)) { + $this->differences[$context] = $levels; + } else { + foreach ($levels as $level => $differences) { + $this->differences[$context][$level] = array_merge($this->differences[$context][$level], $differences); + } + } + } + + return $this; + } +} diff --git a/src/ReportBuilder.php b/src/ReportBuilder.php index bf6e785c..3e7f93d9 100644 --- a/src/ReportBuilder.php +++ b/src/ReportBuilder.php @@ -44,9 +44,9 @@ class ReportBuilder /** * Define analyzer factory list for the different report types. - * @var array + * @var string[] */ - private $analyzerList = [ + private $analyzerFactoryClasses = [ ReportTypes::API => AnalyzerFactory::class, ReportTypes::ALL => NonApiAnalyzerFactory::class, ReportTypes::DB_SCHEMA => DbSchemaAnalyzerFactory::class, @@ -121,11 +121,11 @@ protected function makeVersionReport() /** * Get the mapping of report type -> analyzer factory * - * @return array + * @return string[] */ - protected function getAnalyzerFactories() + protected function getAnalyzerFactoryClasses() { - return $this->analyzerList; + return $this->analyzerFactoryClasses; } /** @@ -171,7 +171,7 @@ protected function buildReport() /** * @var AnalyzerFactoryInterface $factory */ - foreach ($this->getAnalyzerFactories() as $reportType => $factory) { + foreach ($this->getAnalyzerFactoryClasses() as $reportType => $factory) { /** @var AnalyzerInterface $analyzer */ $analyzer = (new $factory())->create($dependencyMap); $tmpReport = $analyzer->analyze( diff --git a/src/Reporter/BreakingChangeTableReporter.php b/src/Reporter/BreakingChangeTableReporter.php index 65419140..3ae55e7f 100644 --- a/src/Reporter/BreakingChangeTableReporter.php +++ b/src/Reporter/BreakingChangeTableReporter.php @@ -7,7 +7,6 @@ // @codingStandardsIgnoreFile namespace Magento\SemanticVersionChecker\Reporter; -use Magento\SemanticVersionChecker\DbSchemaReport; use PHPSemVerChecker\Report\Report; use PHPSemVerChecker\SemanticVersioning\Level; use Symfony\Component\Console\Output\OutputInterface; @@ -52,14 +51,15 @@ public function __construct($apiChangeReport, $apiMembershipReport, $targetFile) */ public function output(OutputInterface $output) { - $contexts = DbSchemaReport::$contexts; + $reportContexts = array_keys($this->report->getDifferences()); + $membershipContexts = array_keys($this->membershipReport->getDifferences()); - foreach ($contexts as $context) { + foreach ($reportContexts as $context) { $header = static::formatSectionHeader($this->targetFile, $context, 'breaking-change'); $this->outputChangeReport($output, $this->report, $context, $header); } - foreach ($contexts as $context) { + foreach ($membershipContexts as $context) { $header = static::formatSectionHeader($this->targetFile, $context, 'api-membership'); $this->outputChangeReport($output, $this->membershipReport, $context, $header); } diff --git a/tests/Unit/MergedReportTest.php b/tests/Unit/MergedReportTest.php new file mode 100644 index 00000000..d2146445 --- /dev/null +++ b/tests/Unit/MergedReportTest.php @@ -0,0 +1,96 @@ +createMock(ClassConstructorOptionalParameterAdded::class)]; + $diffsWithDatabase['class'][Level::MINOR] = [ + $this->createMock(ClassExtendsAdded::class), + $this->createMock(ClassMethodOptionalParameterAdded::class) + ]; + $diffsWithDatabase['database'][Level::MAJOR] = [$this->createMock(DropForeignKey::class)]; + $databaseReport = new InjectableReport($diffsWithDatabase); + + $diffsWithXml = []; + $diffsWithXml['class'][Level::MAJOR] = [$this->createMock(ClassConstantRemoved::class)]; + $diffsWithXml['class'][Level::MINOR] = [$this->createMock(ClassImplementsAdded::class)]; + $diffsWithXml['xml'][Level::MINOR] = [$this->createMock(FieldAdded::class)]; + $xmlReport = new InjectableReport($diffsWithXml); + + /** @var MockObject|ClassTraitAdded $preMergeOperation */ + $preMergeOperation = $this->createMock(ClassTraitAdded::class); + $preMergeOperation->expects($this->any())->method('getLevel')->willReturn(Level::MINOR); + + $mergedReport = new MergedReport(); + $mergedReport->addClass($preMergeOperation); + $mergedReport->merge($databaseReport); + $mergedReport->merge($xmlReport); + + $mergedDiffs = $mergedReport->getDifferences(); + $this->assertTrue(key_exists('database', $mergedDiffs)); + $this->assertTrue(key_exists('xml', $mergedDiffs)); + $this->assertTrue($this->hasAllDifferences($mergedReport, $databaseReport)); + $this->assertTrue($this->hasAllDifferences($mergedReport, $xmlReport)); + $this->assertTrue(array_search($preMergeOperation, $mergedDiffs['class'][Level::MINOR]) !== false); + } + + /** + * Checks if a merged report contains all differences in another report + + * @param Report $mergedReport + * @param Report $inputReport + * @return bool + */ + private function hasAllDifferences($mergedReport, $inputReport) + { + $mergedDiffs = $mergedReport->getDifferences(); + $inputDiffs = $inputReport->getDifferences(); + foreach ($inputDiffs as $context => $levels) { + if (!key_exists($context, $mergedDiffs)) { + return false; + } + $mergedLevels = $mergedDiffs[$context]; + foreach ($levels as $level => $operations) { + if (!key_exists($level, $mergedLevels)) { + return false; + } + $mergedOperations = $mergedLevels[$level]; + foreach ($operations as $operation) { + if (array_search($operation, $mergedOperations) === false) { + return false; + } + } + } + } + return true; + } +}