-
Notifications
You must be signed in to change notification settings - Fork 5
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
Issue #6 2.x - Drop support for PHP <= 8.1 #8
base: 2.x
Are you sure you want to change the base?
Conversation
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 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.
"ext-ctype": "*" | ||
}, | ||
"require-dev": { | ||
"phpunit/phpunit": "^8.5", | ||
"squizlabs/php_codesniffer": "^3.9" | ||
"jetbrains/phpstorm-attributes": "^1.0", |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above there still is a $line[0] === '#'
and both cases could be str_starts_with
, which is now used in other parts
*/ | ||
public function getPackage($name, $version = null) | ||
public function getPackage(string $name, string $version = null): ?Package |
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.
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
{ | ||
$rootPackages = [ | ||
$this->yarnLock->getPackage('lodash', '^4.16.2'), | ||
$this->yarnLock->getPackage('jest-cli', '15.1.1'), | ||
]; | ||
$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'; |
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.
inconsistent operator spacing
[![CircleCI](https://circleci.com/gh/sweetchuck/mindscreen-yarnlock/tree/2.x.svg?style=svg)](https://circleci.com/gh/sweetchuck/mindscreen-yarnlock/?branch=2.x) | ||
[![codecov](https://codecov.io/gh/sweetchuck/mindscreen-yarnlock/branch/2.x/graph/badge.svg?token=HSF16OGPyr)](https://app.codecov.io/gh/sweetchuck/mindscreen-yarnlock/branch/2.x) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with PHP 8 we could use WeakMap instead of the object IDs, which seems cleaner to me
Issue #6 2.x - Drop support for PHP <= 8.1