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.

Sign up for GitHub

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

Jump to bottom

feat: introduce first_opt(), first_key_opt(), last_opt(), last_key_opt() and search_opt() #467

Merged
merged 4 commits into from
May 5, 2024
Merged

feat: introduce first_opt(), first_key_opt(), last_opt(), last_key_opt() and search_opt() #467

merged 4 commits into from
May 5, 2024

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Apr 18, 2024

I'd like to modify search operations over iterables by wrapping the result in Option.

Currently, it is not possible to use these functions to find null value since null represents "no result".

This applies to all first*, last* and search functions.

WDYT?

@azjezz
Copy link
Owner

azjezz commented Apr 19, 2024 •
edited
Loading

I like the idea of returning an Option, but i dislike having this type of BC, what about adding new functions that result in an Option? e.g first_opt<T>(iterable<T> $i): Option<T> vs first<T>(iterable<T> $I): null|T?

_opt suffix is one way to go about this ( this is a popular choice in the rust community, e.g: https://docs.rs/chrono/latest/chrono/struct.Date.html#method.and_hms_micro VS https://docs.rs/chrono/latest/chrono/struct.Date.html#method.and_hms_micro_opt), but i'm open to suggestions if you have any, maybe we can find a more suitable naming convention for those functions.

cc @veewee this might interest you.

@simPod
Copy link
Contributor Author

simPod commented Apr 19, 2024

I'll go for it. Thanks for input.

@veewee
Copy link
Collaborator

veewee commented Apr 19, 2024

@azjezz

The migration path might be better this way indeed.
However you'll probably end up to something that looks like this in the long run, which might be something we'dd want to avoid?

  • v3 -> deprecate old API in favour for _opt
  • v4 -> remove old API
  • v5 -> Recreate original function without the _opt suffix and deprecate the _opt suffix functions
  • v6 -> remove the _opt functions again

@azjezz
Copy link
Owner

azjezz commented Apr 19, 2024

@veewee personally i would like to keep both, i don't see why we need to deprecate/remove the old behaviour.

@simPod simPod changed the title RFC wrap iterable search with Option feat: introduce first_opt(), first_key_opt(), last_opt(), last_key_opt() and search_opt() Apr 27, 2024
@coveralls
Copy link

coveralls commented Apr 27, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8953186695

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 18 of 18 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 98.792%
Totals Coverage Status
Change from base Build 8832169884: 0.01%
Covered Lines: 4415
Relevant Lines: 4469
💛 - Coveralls

@simPod simPod marked this pull request as ready for review April 27, 2024 18:01
azjezz
azjezz requested changes Apr 27, 2024
View reviewed changes
src/Psl/Iter/last_key_opt.php Outdated Show resolved Hide resolved
src/Psl/Iter/last_key_opt.php Outdated Show resolved Hide resolved
src/Psl/Iter/last_opt.php Outdated Show resolved Hide resolved
@simPod simPod requested a review from azjezz May 3, 2024 11:05
azjezz
azjezz requested changes May 3, 2024
View reviewed changes
src/Psl/Iter/last_opt.php Outdated Show resolved Hide resolved
src/Psl/Iter/search_opt.php Outdated Show resolved Hide resolved
simPod and others added 2 commits May 3, 2024 22:41
@simPod @azjezz
Update src/Psl/Iter/last_opt.php
0750c69 
Co-authored-by: Saif Eddin Gmati <29315886+azjezz@users.noreply.github.com>
@simPod
Update src/Psl/Iter/search_opt.php
465bdf0
@simPod simPod requested a review from azjezz May 3, 2024 20:42
azjezz
azjezz requested changes May 4, 2024
View reviewed changes
Copy link
Owner

@azjezz azjezz left a comment

Choose a reason for hiding this comment

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

one last thing 😅

src/Psl/Iter/search_opt.php Outdated
Comment on lines 15 to 21
* Examples:
*
* Iter\search(['foo', 'bar', 'baz'], fn($v) => 'baz' === $v)
* => Str('baz')
*
* Iter\search(['foo', 'bar', 'baz'], fn($v) => 'qux' === $v)
* => Null
Copy link
Owner

Choose a reason for hiding this comment

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

exampless are outdated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. is it correct now? Not sure what Str('baz') was, replaced.

@azjezz azjezz self-assigned this May 4, 2024
@azjezz azjezz added the Type: Enhancement Most issues will probably ask for additions or changes. label May 4, 2024
@azjezz azjezz added this to the 3.0.0 milestone May 4, 2024
@azjezz azjezz added Priority: Medium This issue may be useful, and needs some attention. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness labels May 4, 2024
@azjezz azjezz requested a review from veewee May 4, 2024 21:19
veewee
veewee approved these changes May 5, 2024
View reviewed changes
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.

LGTM. Thanks!

@veewee veewee merged commit 5e4e0ef into azjezz:next May 5, 2024
14 checks passed
@simPod simPod deleted the first-option branch May 5, 2024 09:07
simPod added a commit to simPod/psl that referenced this pull request May 6, 2024
@simPod @azjezz
feat: introduce first_opt(), first_key_opt(), last_opt(), `last…
565abc1 
…_key_opt()` and `search_opt()` (azjezz#467)

* feat: introduce `first_opt()`, `first_key_opt()`, `last_opt()`, `last_key_opt()` and `search_opt()`

* Update src/Psl/Iter/last_opt.php

Co-authored-by: Saif Eddin Gmati <29315886+azjezz@users.noreply.github.com>

* Update src/Psl/Iter/search_opt.php

* example

---------

Co-authored-by: Saif Eddin Gmati <29315886+azjezz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azjezz azjezz azjezz approved these changes

@veewee veewee veewee approved these changes

Assignees

@azjezz azjezz

Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@simPod @azjezz @veewee @coveralls