Issue #6 2.x - Drop support for PHP <= 8.1#8
Issue #6 2.x - Drop support for PHP <= 8.1#8Sweetchuck wants to merge 1 commit intomindscreen:2.xfrom
Conversation
PRGfx
left a comment
PRGfx
left a comment
There was a problem hiding this comment.
I appreciate the clean up and introduction of new language features.
However, some things seem mostly to be based on personal preference and I don't see why to restructure the code based on this (e.g. I'm not a fan of trailing commas on parameter lists or mixing class properties and accessors).
The interface seems barely changed, so maybe you could outline your changes and motivate them, so we can focus on that before nitpicking code-style.
| "require-dev": { | ||
| "phpunit/phpunit": "^8.5", | ||
| "squizlabs/php_codesniffer": "^3.9" | ||
| "jetbrains/phpstorm-attributes": "^1.0", |
There was a problem hiding this comment.
Might need specific testing, but I had issues with reflection in the past, when a library did not provide the attributes they were using.
| - .phpstan/parameters.ignoreErrors.baseline.neon | ||
| - .phpstan/parameters.ignoreErrors.custom.neon | ||
| - .phpstan/parameters.typeAliases.prod.neon | ||
| - .phpstan/parameters.typeAliases.dev.neon |
There was a problem hiding this comment.
seems a bit much to me for this project, if it's mostly for the sake of best practice and the files are "empty".
|
|
||
| namespace Mindscreen\YarnLock; | ||
|
|
||
| enum ParserErrorCode: int |
There was a problem hiding this comment.
I like the benefit of "uniquely" tracing an error to a point in code. At this point 1..6 x exception class might still be unique, but depending on error handling, the original class name migth get lost.
Maybe this is less of an issue in such library code, which probably isn't inspected by the user, but to me it makes sense.
| } | ||
| if ($line[0] === '#') { | ||
|
|
||
| if (substr($line, 0, 1) === '#') { |
There was a problem hiding this comment.
above there still is a $line[0] === '#' and both cases could be str_starts_with, which is now used in other parts
| * @return Package|null | ||
| */ | ||
| public function getPackage($name, $version = null) | ||
| public function getPackage(string $name, string $version = null): ?Package |
There was a problem hiding this comment.
imo the version could be nullable. It is a case handled by the code and there are scenarios where it is easier to call with a nullable value rather than maybe omitting the parameter
| $this->yarnLock->calculateDepth($rootPackages); | ||
| $depth0 = $this->yarnLock->getPackagesByDepth(0); | ||
| static::assertSame(count($rootPackages), count($depth0)); | ||
| static::assertEquals(count($rootPackages), count($depth0)); |
There was a problem hiding this comment.
Why would you change this but not the next one?
| case ProdRequired = 'dependencies'; | ||
|
|
||
| case ProdOptional ='optionalDependencies'; | ||
|
|
||
| case PeerRequired = 'peerDependencies'; | ||
|
|
||
| case DevRequired = 'devDependencies'; |
There was a problem hiding this comment.
I see how "dependencies" is very "broad", as there are in fact sub-types.
However, these are - as far as I see - the terms used by yarn.
Also I don't see the need for a distinction of "required" and "optional", if "optional" is its own singular kind, i.e. there are no optional dev dependencies.
I see "production" as option on the installation command. Do you have another source?
I would believe that this is mostly based on "dependencies" before the introduction of any additional kind. Depending on the context one could differentiate between "build-time" and "run-time" or other terms as well, so I don't like introducing "unofficial" terms.
This topic extends to the getters in the Package.
| { | ||
| case ProdRequired = 'dependencies'; | ||
|
|
||
| case ProdOptional ='optionalDependencies'; |
| [](https://circleci.com/gh/sweetchuck/mindscreen-yarnlock/?branch=2.x) | ||
| [](https://app.codecov.io/gh/sweetchuck/mindscreen-yarnlock/branch/2.x) |
There was a problem hiding this comment.
reminder to change this back in the end
| @@ -72,17 +69,13 @@ protected static function evaluatePackages($data) | |||
| if (!empty($handledPackages[spl_object_id($package)])) { | |||
There was a problem hiding this comment.
with PHP 8 we could use WeakMap instead of the object IDs, which seems cleaner to me
2 participants
Issue #6 2.x - Drop support for PHP <= 8.1