Skip to content

Add Indexable#find and #find!#15552

Merged
straight-shoota merged 10 commits intocrystal-lang:masterfrom
punteek:findWithOffsets
Mar 19, 2025
Merged

Add Indexable#find and #find!#15552
straight-shoota merged 10 commits intocrystal-lang:masterfrom
punteek:findWithOffsets

Conversation

@punteek
Copy link
Contributor

@punteek punteek commented Mar 13, 2025

Added implementation and tests for find and find! in Indexable, as discussed in this issue: #14642

@HertzDevil
Copy link
Contributor

HertzDevil commented Mar 14, 2025

This failure:

Failures:

  1) Enumerable find doesn't find with default value
     Failure/Error: [1, 2, 3].find(-1) { |x| x > 3 }.should eq(-1)

       Expected: -1
            got: nil

     # spec/std/enumerable_spec.cr:545

is because it is now calling Indexable#find(offset : Int = 0, if_none = nil, & : T ->), i.e.

Find an element greater than 3 starting from offset -1, otherwise return nil,

instead of the existing Enumerable#find(if_none = nil, & : T ->), i.e.

Find an element greater than 3 starting from the first element, otherwise return -1.

offset as a positional parameter should go after if_none.

@Fryguy
Copy link
Contributor

Fryguy commented Mar 14, 2025

With 2 integers as positional parameters, it feels hard to read...I'm wondering if offset should be a forced keyword arg.

@Sija
Copy link
Contributor

Sija commented Mar 14, 2025

@Fryguy There's just one: offset, if_none has an arbitrary type.

@punteek
Copy link
Contributor Author

punteek commented Mar 14, 2025

re-ordered the parameters

@punteek
Copy link
Contributor Author

punteek commented Mar 15, 2025

Are there any other suggestions to this PR?

Copy link
Contributor

@Sija Sija left a comment

Choose a reason for hiding this comment

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

LGTM ❤️

@punteek
Copy link
Contributor Author

punteek commented Mar 15, 2025

Couple of questions on the rest of the process for PR check in:

  1. Do the MinGW-w64 CI failing checks need to be passing for this PR to be merged?
  2. Once all checks are passing and there are 2 approvers, does the PR automatically merge? Or will someone manually merge this branch in?
  3. What does the process look like to add the new find and find! methods to documentation? Ideally that update would come alongside merging this PR

Thanks!

@Sija
Copy link
Contributor

Sija commented Mar 15, 2025

  1. Do the MinGW-w64 CI failing checks need to be passing for this PR to be merged?

I don't think so, since these failures are unrelated to this PR.

  1. Once all checks are passing and there are 2 approvers, does the PR automatically merge? Or will someone manually merge this branch in?

Merging is a manual process. Mind you, only approvals from the team members are counted towards the required number (and I'm not part of the Core Team).

  1. What does the process look like to add the new find and find! methods to documentation? Ideally that update would come alongside merging this PR

Documentation is auto-generated based on the public API docs, so there's nothing more to do (as you've already documented the newly added methods).

@straight-shoota straight-shoota changed the title Add find and find! methods to Indexable Add Indexable#find and #find! Mar 17, 2025
@straight-shoota straight-shoota added this to the 1.16.0 milestone Mar 17, 2025
@straight-shoota straight-shoota merged commit 976507c into crystal-lang:master Mar 19, 2025
35 of 38 checks passed
@straight-shoota straight-shoota linked an issue Oct 16, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Indexable#find and #find! with start offsets

6 participants