Skip to content
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

Introduce a comparison component #428

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Introduce a comparison component #428

merged 1 commit into from
Nov 22, 2023

Conversation

veewee
Copy link
Collaborator

@veewee veewee commented Oct 25, 2023

This PR introduces a comparison component:

  • It provides sensible interfaces for adding a compare and equal function to your own classes:
    • Comparable<T>
    • Equable<T>
  • It provide functions for comparing values of the same (base)type:
    • compare(T $a, T $b): Order
    • equal(T $a, T $b): bool
    • not_equal(T $a, T $b): bool
    • less(T $a, T $b): bool
    • less_or_equal(T $a, T $b): bool
    • greater(T $a, T $b): bool
    • greater_or_equal(T $a, T $b): bool
    • sort(T $a, T $b): -1 | 0 | 1 (as shortcut for e.g. vec sorting)
  • Comparison of objects that implement Comparable is done based on the internal logic of that specific class. An implementation for the Option component was added.
  • Comparison of other types is being done based on PHPs <=> comparison operator
  • In case you want to make 2 types incomparable, a user can trhow a IncomparableException
  • It's fully typesafe until you end up with mixed. (At that moment, PHPs default compare system kicks in. A user could opt-in on throwing an exception manually in the class that implements Comparable)
Some examples

OPTION

/**
 * @template T
 *
 * @implements Comparison\Comparable<Option<T>>
 * @implements Comparison\Equable<Option<T>>
 */
final class Option implements Comparison\Comparable, Comparison\Equable
{
    /**
     * @param Option<T> $other
     */
    public function compare(mixed $other): Comparison\Order
    {
        $aIsNone = $this->isNone();
        $bIsNone = $other->isNone();

        return match (true) {
            $aIsNone || $bIsNone => Comparison\compare($bIsNone, $aIsNone),
            default => Comparison\compare($this->unwrap(), $other->unwrap())
        };
    }

    /**
     * @param Option<T> $other
     */
    public function equal(mixed $other): bool
    {
        return Comparison\equal($this, $other);
    }
}

COVARIANCE

/**
 * @implements Comparable<Size>
 */
abstract class Size implements Comparable
{
    abstract public function normalizedValue(): int;

    public function compare(mixed $other): Order
    {
        return Comparison\compare($this->normalizedValue(), $other->normalizedValue());
    }
}

class Inches extends Size
{
    public function normalizedValue(): int
    {
        return 1;
    }
}

class Centimeters extends Size
{
    public function normalizedValue(): int
    {
        return 2;
    }
}


function test_covariant_limitations(): Order
{
    $cm = new Centimeters();
    $inch = new Inches();

    return $cm->compare($inch);
}

MIXED

I've tried throwing a lot of different types at the <=> operator and wasn't able to make PHP throw exceptions.
Therefore, any non Comparable object falls back to PHPs internal sorting system.

As long as I cannot find incomparable values, I won't enforce having to deal with IncomparableException from the API.

function test_mixed_limitations(mixed $a, mixed $b): Order
{
    return Comparison\compare($a, $b);
}

(The mutation test issues are solved in #427)

@veewee veewee added Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix Status: Review Needed The issue has a PR attached to it which needs to be reviewed. Type: Enhancement Most issues will probably ask for additions or changes. labels Oct 25, 2023
@veewee veewee requested a review from azjezz October 25, 2023 14:33
@coveralls
Copy link

coveralls commented Oct 25, 2023

Pull Request Test Coverage Report for Build 6953644254

  • 27 of 27 (100.0%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 99.005%

Totals Coverage Status
Change from base Build 6953598542: 0.006%
Covered Lines: 4179
Relevant Lines: 4221

💛 - Coveralls

*/
function equal(mixed $a, mixed $b): bool
{
return compare($a, $b) === Order::Equal;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about adding a Equable check here and use the Equable::equals() method if possible. This could work for equal and not equal.
However, it would mean adding additional comparisons for less_or_equal and greater_or_equal. Not sure if it's a good idea and worth adding.

@veewee veewee added this to the 2.8.0 milestone Nov 22, 2023
@veewee veewee added Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. and removed Status: Review Needed The issue has a PR attached to it which needs to be reviewed. labels Nov 22, 2023
@veewee veewee merged commit f106d9e into azjezz:next Nov 22, 2023
12 checks passed
renovate bot referenced this pull request in ben-challis/sql-migrations Nov 23, 2023
[![Mend Renovate logo
banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [azjezz/psl](https://togithub.com/azjezz/psl) | `2.7.0` -> `2.8.0` |
[![age](https://developer.mend.io/api/mc/badges/age/packagist/azjezz%2fpsl/2.8.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/azjezz%2fpsl/2.8.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/azjezz%2fpsl/2.7.0/2.8.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/azjezz%2fpsl/2.7.0/2.8.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>azjezz/psl (azjezz/psl)</summary>

### [`v2.8.0`](https://togithub.com/azjezz/psl/releases/tag/2.8.0):
Lenalee - 2.8.0

[Compare Source](https://togithub.com/azjezz/psl/compare/2.7.0...2.8.0)

#### What's Changed

- chore(ga): bump actions/checkout from 3 to 4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/azjezz/psl/pull/420](https://togithub.com/azjezz/psl/pull/420)
- Fix mutations + math float tests by
[@&#8203;veewee](https://togithub.com/veewee) in
[https://github.com/azjezz/psl/pull/427](https://togithub.com/azjezz/psl/pull/427)
- Introduce a comparison component by
[@&#8203;veewee](https://togithub.com/veewee) in
[https://github.com/azjezz/psl/pull/428](https://togithub.com/azjezz/psl/pull/428)
- Indicate support for PHP 8.3 by
[@&#8203;gsteel](https://togithub.com/gsteel) in
[https://github.com/azjezz/psl/pull/430](https://togithub.com/azjezz/psl/pull/430)

#### New Contributors

- [@&#8203;gsteel](https://togithub.com/gsteel) made their first
contribution in
[https://github.com/azjezz/psl/pull/430](https://togithub.com/azjezz/psl/pull/430)

**Full Changelog**: azjezz/psl@2.7.0...2.8.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/ben-challis/sql-migrations).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy41OS44IiwidXBkYXRlZEluVmVyIjoiMzcuNTkuOCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants