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

feat(option): new Option::apply() method #426

Merged
merged 7 commits into from
Dec 29, 2023
Merged

Conversation

devnix
Copy link
Contributor

@devnix devnix commented Oct 18, 2023

I guess it's not an important method because you can return the same value with map, but I'll leave the proposal 😄

@coveralls
Copy link

coveralls commented Oct 18, 2023

Pull Request Test Coverage Report for Build 7355771608

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.012%

Totals Coverage Status
Change from base Build 7354836553: 0.0%
Covered Lines: 4211
Relevant Lines: 4253

💛 - Coveralls

@veewee
Copy link
Collaborator

veewee commented Oct 25, 2023

What would be the big benifit of this over using map()?
Would you consume it and chain it back into doing another operation?

I don't see how I'dd use it personally at this moment, but I'm open for opinions :)

@devnix
Copy link
Contributor Author

devnix commented Oct 28, 2023

Just syntactic sugar for operations if it holds some, but avoiding worrying about the verbosity of returning the same value if you want to chain methods.

Code like this (probably not the best example)

$stock
    ->map(function (int $value) use ($logger) {
        $logger->info('New stock requested', ['stock' => $value]);
        return $value;
    })
    ->map(Stock::create(...))
    ->map(function (Stock $value) use ($product) {
        $product->updateStock($value);
        return $value;
    });

could be much more readable, and probably more expressive about intention:

$stock
    ->apply(fn (int $value) => $logger->info('New stock requested', ['stock' => $value]))
    ->map(Stock::create(...))
    ->apply(fn (Stock $value) => $product->updateStock($stock));

@veewee
Copy link
Collaborator

veewee commented Nov 22, 2023

@devnix can you rebase against the next branch to get the github actions green again and add some tests?
It could make sense for people to have this option.

@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: Revision Needed At least two people have seen issues in the PR that makes them uneasy. Type: Enhancement Most issues will probably ask for additions or changes. labels Nov 22, 2023
Copy link
Collaborator

@veewee veewee left a comment

Choose a reason for hiding this comment

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

Looks good thanks. I've added a small couple of nitpicks to the code review.
After those it should be good to go!

tests/unit/Option/NoneTest.php Outdated Show resolved Hide resolved
tests/unit/Option/NoneTest.php Outdated Show resolved Hide resolved
tests/unit/Option/SomeTest.php Outdated Show resolved Hide resolved
@devnix devnix requested a review from veewee December 11, 2023 18:59
@devnix devnix requested a review from veewee December 12, 2023 15:36
@devnix
Copy link
Contributor Author

devnix commented Dec 29, 2023

@veewee I think this should be ready too, conflicts are resolved

@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: Revision Needed At least two people have seen issues in the PR that makes them uneasy. labels Dec 29, 2023
@veewee veewee merged commit a8685b2 into azjezz:next Dec 29, 2023
14 checks passed
@veewee
Copy link
Collaborator

veewee commented Dec 29, 2023

Cool, Thanks again!

@devnix devnix deleted the option-apply branch December 29, 2023 10:27
renovate bot referenced this pull request in ben-challis/sql-migrations Dec 29, 2023
[![Mend
Renovate](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.8.0` -> `2.9.0` |
[![age](https://developer.mend.io/api/mc/badges/age/packagist/azjezz%2fpsl/2.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/azjezz%2fpsl/2.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/azjezz%2fpsl/2.8.0/2.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/azjezz%2fpsl/2.8.0/2.9.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.9.0`](https://togithub.com/azjezz/psl/releases/tag/2.9.0):
Lenalee - 2.9.0

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

#### What's Changed

- Apply fixes for Psalm 5.17 by
[@&#8203;veewee](https://togithub.com/veewee) in
[https://github.com/azjezz/psl/pull/431](https://togithub.com/azjezz/psl/pull/431)
- feat(type): add class_string types
([#&#8203;432](https://togithub.com/azjezz/psl/issues/432)) by
[@&#8203;zerkms](https://togithub.com/zerkms) in
[https://github.com/azjezz/psl/pull/435](https://togithub.com/azjezz/psl/pull/435)
- feat(option): add `Option::zip()`, `Option::zipWith()` and
`Option::unzip()` methods by
[@&#8203;devnix](https://togithub.com/devnix) in
[https://github.com/azjezz/psl/pull/434](https://togithub.com/azjezz/psl/pull/434)
- feat(option): add `Option::proceed()` method by
[@&#8203;devnix](https://togithub.com/devnix) in
[https://github.com/azjezz/psl/pull/433](https://togithub.com/azjezz/psl/pull/433)
- feat(option): new `Option::apply()` method by
[@&#8203;devnix](https://togithub.com/devnix) in
[https://github.com/azjezz/psl/pull/426](https://togithub.com/azjezz/psl/pull/426)

#### New Contributors

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

**Full Changelog**: azjezz/psl@2.8.0...2.9.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:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMDMuMSIsInVwZGF0ZWRJblZlciI6IjM3LjEwMy4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

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.

3 participants