strong typing, refactoring #1596
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1596 +/- ##
===========================================
+ Coverage 44.75% 45.06% +0.3%
- Complexity 753 791 +38
===========================================
Files 1542 1553 +11
Lines 52276 52418 +142
Branches 21305 21305
===========================================
+ Hits 23397 23623 +226
+ Misses 28879 28795 -84
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1596 +/- ##
============================================
+ Coverage 46.74% 46.98% +0.23%
+ Complexity 776 711 -65
============================================
Files 1569 1575 +6
Lines 53606 53491 -115
Branches 21845 21845
============================================
+ Hits 25060 25132 +72
+ Misses 28546 28359 -187
Continue to review full report at Codecov.
|
|
@mimmi20 is this ready to be looked at? |
|
Yes |
src/Browscap/Data/DataCollection.php
Outdated
| $json = json_decode($fileContent, true); | ||
|
|
||
| if (null === $json) { | ||
| throw new \RuntimeException('File "' . $filename . '" had invalid JSON.'); |
There was a problem hiding this comment.
It may be helpful here to include the JSON error triggered by the json_decode call in this exception message: http://php.net/manual/en/function.json-last-error-msg.php
There was a problem hiding this comment.
error message is added now
| /** | ||
| * @param array $useragentData | ||
| * @param array $versions | ||
| * @param array %$allDivisions |
There was a problem hiding this comment.
I couldn't find documentation of this, but I assume the % here is to indicate that the param is passed by reference?
There was a problem hiding this comment.
It should have been a &, but it is is removed now.
| } | ||
|
|
||
| if (false === mb_strpos($useragentData['userAgent'], '#') | ||
| && in_array($useragentData['userAgent'], $allDivisions) |
There was a problem hiding this comment.
And to that note, does $allDivisions need to be passed by reference?
This appears to be the only place it's used in this method, and it shouldn't be getting modified by an in_array call. Am I missing some other use of it here where passing by reference is necessary?
src/Browscap/Filter/FullFilter.php
Outdated
| } | ||
|
|
||
| /** | ||
| * returns the Type of the filter | ||
| * | ||
| * @return string | ||
| */ | ||
| public function getType() | ||
| public function getType() : string | ||
| { | ||
| return 'FULL'; |
There was a problem hiding this comment.
You turned the csv, ini, asp strings into constants on the FormatterInterface class, should a similar thing be done with these strings as well?
| { | ||
| return [ |
There was a problem hiding this comment.
Why was this data provider removed? Are these values checked elsewhere?
There was a problem hiding this comment.
This function returns true independend of the value of the parameters. So I dont think we need a test provider.
src/Browscap/Filter/CustomFilter.php
Outdated
| } | ||
|
|
||
| /** | ||
| * returns the Type of the filter | ||
| * | ||
| * @return string | ||
| */ | ||
| public function getType() | ||
| public function getType() : string | ||
| { | ||
| return 'CUSTOM'; |
There was a problem hiding this comment.
Perhaps another case where a constant should be used?
src/Browscap/Coverage/Processor.php
Outdated
| @@ -80,7 +80,7 @@ | |||
| * A storage variable of the pattern ids covered by tests for a specific file (set when processing of that | |||
| * file begins) | |||
| * | |||
| * @var string[] | |||
| * @var array | |||
There was a problem hiding this comment.
I'd rather be more specific about the contents of this: string[] is better (if that's what it is)
| @@ -42,6 +41,8 @@ class BuildCommand extends Command | |||
|
|
|||
| /** | |||
| * Configures the current command. | |||
| * | |||
| * @return void | |||
There was a problem hiding this comment.
Don't need this phpdoc at all really
| $input->getOption('resources'), | ||
| $buildFolder | ||
| ); | ||
| $version = $input->getArgument('version'); |
There was a problem hiding this comment.
Why declare this? It doesn't seem to be used anywhere except on L102..
src/Browscap/Data/DataCollection.php
Outdated
| /** | ||
| * @return \Psr\Log\LoggerInterface $logger | ||
| * @param string $version | ||
| * @param \Psr\Log\LoggerInterface $logger |
There was a problem hiding this comment.
Don't need to use fully qualified class name here (LoggerInterface is already imported)
src/Browscap/Data/DataCollection.php
Outdated
| { | ||
| return $this->logger; | ||
| $this->logger = $logger; | ||
| $this->generationDate = new \DateTime(); |
There was a problem hiding this comment.
Could we use \DateTimeImmutable? 🤔
src/Browscap/Filter/CustomFilter.php
Outdated
| } | ||
|
|
||
| /** | ||
| * returns the Type of the filter | ||
| * | ||
| * @return string | ||
| */ | ||
| public function getType() | ||
| public function getType() : string | ||
| { | ||
| return 'CUSTOM'; |
src/Browscap/Filter/LiteFilter.php
Outdated
| @@ -83,7 +79,7 @@ public function isOutputSection(array $section) | |||
| * | |||
| * @return bool | |||
| */ | |||
| public function isOutputProperty($property, ?WriterInterface $writer = null) | |||
| public function isOutputProperty(string $property, ?WriterInterface $writer = null) : bool | |||
There was a problem hiding this comment.
Ideally squash out optional deps; if it's not "required" just create a DummyWriter which does nothing for example
| $this->buildFolder = $this->checkDirectoryExists($buildFolder); | ||
| $this->logger = $logger; | ||
| $this->writerCollection = $writerCollection; | ||
| $this->collectionCreator = new CollectionCreator($logger); |
There was a problem hiding this comment.
Should also be injected (even if optional with this as default)
| $realDirectory = realpath($directory); | ||
|
|
||
| if (false === $realDirectory) { | ||
| throw new \Exception('The directory "' . $directory . '" does not exist, or we cannot access it'); |
There was a problem hiding this comment.
Throw a more specific exception please (even \RuntimeException is better)
| } | ||
|
|
||
| if (!is_dir($realDirectory)) { | ||
| throw new \Exception('The path "' . $realDirectory . '" did not resolve to a directory'); |
There was a problem hiding this comment.
Throw a more specific exception please (even \RuntimeException is better)
|
@asgrim Where should the BC breaks be documented? Readme.md? |
|
Yes please |
|
What is the public API of this project? Currently there's no mention in the docs of how to use the code in this project outside of the CLI commands (unless I missed something), though this PR does add some code examples for custom builds. Since this project's primary purpose is to build the data files (and this is primarily done through CLI commands or the integration tests) should there be much of a public API, and if so, shouldn't that be documented? |
|
The public API is basically only used by the website to generate the files, AFAIK; mainly big breaks should be documented for my sanity ;) |
|
@jaydiablo @asgrim Please review again. |
README.md
Outdated
|
|
||
| ## Changes | ||
|
|
||
| * The tests for integration testing the source files are splitted from the other tests |
There was a problem hiding this comment.
splitted should be split
README.md
Outdated
| ## Changes | ||
|
|
||
| * The tests for integration testing the source files are splitted from the other tests | ||
| * Tests on travis use the build pipiline now |
There was a problem hiding this comment.
Type-o on pipiline here (should be pipeline)
README.md
Outdated
| the CLI commands | ||
| ---------------- | ||
|
|
||
| There is actaully only one cli command available. |
There was a problem hiding this comment.
Another type-o here (actually).
README.md
Outdated
| - `version` (required) the name of the version that should be built | ||
| - `output` (optional) the directory where the files should be created | ||
| - `resources` (optional) the directory where the sources for the build are located | ||
| - `coverage` (optional) if this option is set, during the build a coverage file is created |
There was a problem hiding this comment.
This description isn't entirely correct.
By specifying this param, the pattern ids used for coverage reporting are added to the INI files, but no coverage is actually generated (since tests have to run to collect coverage info).
| declare(strict_types = 1); | ||
| namespace Browscap\Generator; | ||
|
|
||
| class DirectoryMissingException extends \InvalidArgumentException |
There was a problem hiding this comment.
You have this exception class, and "NoDirectoryException" below, don't they mean the same thing?
There was a problem hiding this comment.
the other exception is renamed
| declare(strict_types = 1); | ||
| namespace Browscap\Generator; | ||
|
|
||
| class NoDirectoryException extends \InvalidArgumentException |
There was a problem hiding this comment.
Here's the other exception that I mention above.
| } | ||
|
|
||
| if (!is_dir($realDirectory)) { | ||
| throw new NoDirectoryException('The path "' . $realDirectory . '" did not resolve to a directory'); |
There was a problem hiding this comment.
Ah, I see, here's the usage of this.
I'd name this one "NotADirectoryException" or "NotDirectoryException" or something like that. "NoDirectory" could mean the same thing as the one above.
|
@jaydiablo @asgrim Please review again. |
|
I noticed a couple of small type-os/grammar changes in the readme that I missed earlier. I can comment on those. As far as the rest of the PHP changes, I was going to leave that to @asgrim, since he had requested changes earlier. |
|
Build is ok now. |
|
Perfect, thanks @mimmi20, nice work here :) |
I added strong typing to most of the functions and removed most of the setters and getters and the fluid interfaces. Additional to that I did some refactorings.
Because of the refactorings I had to write more unittests.
This PR is required for PR #1520 and PR #1579.
Fixes #1585
Fixes #1586
Fixes #1587