Enhancement: Extract `Version` #872

localheinz · 2023-12-06T22:47:09Z

This pull request

extracts Version as a value object

Related to #871.

localheinz · 2023-12-06T22:47:35Z

include/branches.inc

-    $parts = explode('.', $version);
-    if (count($parts) > 1) {
-        return "$parts[0].$parts[1]";
+function version_number_to_branch(string $value): ?string {


This function could eventually disappear, and is only a starting point for using Version - also see #871.

Girgias

Just some comments after a quick glance

Girgias · 2023-12-06T23:24:59Z

src/Release/Patch.php

+        if (1 !== preg_match('/^(0|[1-9]\d*)$/', $value)) {
+            throw new \InvalidArgumentException(\sprintf(
+                'Value "%s" does not appear to be a valid value for a patch version.',
+                $value
+            ));
+        }


Use ctype_digit() ?

ctype_digit($value) returns true when $value has a leading 0: https://3v4l.org/XqGiU.

Girgias · 2023-12-06T23:26:08Z

src/Release/Version.php

+    public function major(): Major
+    {
+        return $this->major;
+    }
+
+    public function minor(): Minor
+    {
+        return $this->minor;
+    }
+
+    public function patch(): Patch
+    {
+        return $this->patch;
+    }


Do we really need those if we have readonly properties?

Depends on whether we want to stick with the internal implementation: using accessors instead of public properties has the advantage that we can change the internals any time without changing the consumers.

I can adjust, though.

Girgias · 2023-12-06T23:27:10Z

src/Release/Version.php

+            throw new \InvalidArgumentException(\sprintf(
+                'Value "%s" does not appear to be a valid value for a version.',
+                $value
+            ));


Suggested change

throw new \InvalidArgumentException(\sprintf(

'Value "%s" does not appear to be a valid value for a version.',

$value

));

throw new \ValueError(\sprintf(

'Value "%s" does not appear to be a valid value for a version.',

$value

));

We could just throw a ValueError

kamil-tekiela · 2023-12-06T23:31:56Z

include/branches.inc

-        return "$parts[0].$parts[1]";
+function version_number_to_branch(string $value): ?string {
+    try {
+        $version = Release\Version::fromString($value);


Why not $version = Version::fromString($value); What am I missing?

Adjusted!

I have written about Avoiding imports and aliases in PHP; I generally try to avoid importing every single class from a specific namespace.

Thanks. I am rather against importing the namespace. IMHO every class should be imported unless it conflicts with another one. Only in case of conflicts, one should import namespace. Importing the full name makes for a cleaner code. The line is shorter, there's less visual bloat and my eyes can focus on what's really important: the name of the class. For the most part, I am not interested from which namespace the class comes. I can always hover over it to see the full namespace. The import list is maintained automatically by IDEs so I never look at it. If the import list is too long, it can always be collapsed using the alternative syntax.

kamil-tekiela · 2023-12-06T23:34:04Z

src/Release/Major.php

+    public function toString(): string
+    {
+        return $this->value;
+    }


This method is completely unnecessary and also can be easily confused with the magic method __toString. This is the same as making the property public and directly accessing it.

It's a matter of preference - when I allow constructing an object from a simple type, I typically use a named constructor fromSimpleType() and a corresponding method toSimpleType(). This allows to construct an object from and cast it to other simple types when necessary, and hides the internals.

I have written about Naming constructors in PHP, where I attempt to explain my reasoning.

Also see https://github.com/php/web-php/pull/872/files#r1418528779.

kamil-tekiela · 2023-12-06T23:35:38Z

src/Release/Version.php

+        return $this->patch;
+    }
+
+    public function toString(): string


Why not just use the magic method __toString?

I prefer to explicitly invoke a method to cast a type to another type instead of relying on a magic method.

kamil-tekiela · 2023-12-06T23:36:28Z

src/Release/Version.php

+    public static function create(
+        Major $major,
+        Minor $minor,
+        Patch $patch,
+    ): self {
+        return new self(
+            $major,
+            $minor,
+            $patch,
+        );
+    }


Why not just make the constructor public?

Using one or more public named constructors and a single private constructor in conjunction with accessors instead of public properties allows us to change the internals of the class as needed.

From the call-site it does not matter whether someone invokes a method or accesses a property, but relying on a public property makes it harder to change internals.

For example, we could change the internals as follows and would not need to make any changes to call-sites:

diff --git a/src/Release/Version.php b/src/Release/Version.php index 47975096..6b5664fd 100644 --- a/src/Release/Version.php +++ b/src/Release/Version.php @@ -6,11 +6,9 @@ namespace phpweb\Release; final readonly class Version { - private function __construct( - private Major $major, - private Minor $minor, - private Patch $patch, - ) { + private function __construct(private string $value) + { + } public static function create( @@ -18,11 +16,12 @@ final readonly class Version Minor $minor, Patch $patch, ): self { - return new self( - $major, - $minor, - $patch, - ); + return new self(sprintf( + '%s.%s.%s', + $major->toString(), + $minor->toString(), + $patch->toString(), + )); } /** @@ -37,35 +36,32 @@ final readonly class Version )); } - return new self( - Major::fromString($matches['major']), - Minor::fromString($matches['minor']), - Patch::fromString($matches['patch']), - ); + return new self($value); } public function major(): Major { - return $this->major; + $parts = explode('.', $this->value); + + return Major::fromString($parts[0]); } public function minor(): Minor { - return $this->minor; + $parts = explode('.', $this->value); + + return Minor::fromString($parts[1]); } public function patch(): Patch { - return $this->patch; + $parts = explode('.', $this->value); + + return Patch::fromString($parts[2]); } public function toString(): string { - return sprintf( - '%s.%s.%s', - $this->major->toString(), - $this->minor->toString(), - $this->patch->toString(), - ); + return $this->value; } }

Personally, I prefer the principles of KISS and YAGNI. When it comes to value objects, they should remain as simple as possible. But there's no reason to spend too much time on this as it feels more of a stylistic choice than anything else.

kamil-tekiela · 2023-12-06T23:38:06Z

tests/UserNotes/Release/Major/from-string-rejects-invalid-value.phpt

+$invalidValues = array_filter($values, static function (string $value): bool {
+    try {
+        Release\Major::fromString($value);
+    } catch (\InvalidArgumentException) {
+        return true;
+    }
+
+    return false;
+});


This is too complex. Please simplify it to get rid of the higher-order function.

This is a basic array_filter() usage, I don't see how this is complex.

Could it be just a foreach loop? Sure, but that just a preference at this point.

@Girgias @kamil-tekiela

Do you have any suggestions how to work with data providers in phpt tests?

This is what I am trying to achieve here.

tests/UserNotes/Release/Version/from-string-returns-version.phpt

kamil-tekiela · 2023-12-07T00:03:06Z

tests/UserNotes/Release/Version/from-string-rejects-invalid-value.phpt

+$invalidValues = array_filter($values, static function (string $value): bool {
+    try {
+        Release\Version::fromString($value);
+    } catch (\InvalidArgumentException) {
+        return true;
+    }
+
+    return false;
+});
+
+var_dump($invalidValues);


Suggested change

$invalidValues = array_filter($values, static function (string $value): bool {

try {

Release\Version::fromString($value);

} catch (\InvalidArgumentException) {

return true;

}

return false;

});

var_dump($invalidValues);

foreach($values as $label => $value) {

try {

Release\Version::fromString($value);

} catch (\InvalidArgumentException) {

echo $label . "\n";

echo $value . "\n";

}

}

@Girgias Isn't this easier to read?

@kamil-tekiela @Girgias

I can adjust if you prefer!

localheinz commented Dec 6, 2023

View reviewed changes

localheinz force-pushed the feature/version branch from 7a861a8 to 4b8f0e1 Compare December 6, 2023 22:50

Girgias reviewed Dec 6, 2023

View reviewed changes

kamil-tekiela reviewed Dec 6, 2023

View reviewed changes

tests/UserNotes/Release/Version/from-string-returns-version.phpt Outdated Show resolved Hide resolved

kamil-tekiela reviewed Dec 7, 2023

View reviewed changes

localheinz force-pushed the feature/version branch 2 times, most recently from 7e23de6 to 3787bcb Compare December 7, 2023 08:17

Enhancement: Extract Version

52902ec

localheinz marked this pull request as draft December 7, 2023 14:40

localheinz force-pushed the feature/version branch from 3787bcb to 52902ec Compare December 7, 2023 14:42

localheinz marked this pull request as ready for review December 7, 2023 14:42

saundefined added the Enhancement label Feb 1, 2024

localheinz closed this Feb 12, 2024

localheinz deleted the feature/version branch February 12, 2024 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: Extract `Version` #872

Enhancement: Extract `Version` #872

localheinz commented Dec 6, 2023

localheinz Dec 6, 2023 •

edited

Loading

Girgias left a comment

Girgias Dec 6, 2023

localheinz Dec 7, 2023

Girgias Dec 6, 2023

localheinz Dec 7, 2023

Girgias Dec 6, 2023

kamil-tekiela Dec 6, 2023

localheinz Dec 7, 2023

kamil-tekiela Dec 7, 2023

kamil-tekiela Dec 6, 2023

localheinz Dec 7, 2023

kamil-tekiela Dec 6, 2023

localheinz Dec 7, 2023

kamil-tekiela Dec 6, 2023

localheinz Dec 7, 2023

kamil-tekiela Dec 7, 2023

kamil-tekiela Dec 6, 2023

Girgias Dec 6, 2023

localheinz Dec 7, 2023

kamil-tekiela Dec 7, 2023

localheinz Dec 7, 2023

Enhancement: Extract Version #872

Enhancement: Extract Version #872

Conversation

localheinz commented Dec 6, 2023

localheinz Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enhancement: Extract `Version` #872

Enhancement: Extract `Version` #872

localheinz Dec 6, 2023 •

edited

Loading