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

Implement (try)FindLastSliceIndex #581

Merged
merged 8 commits into from
Jan 6, 2024
Merged

Conversation

rzikm
Copy link
Contributor

@rzikm rzikm commented Jan 3, 2024

Closes #574

I was not able to build the test project, so I rely on CI to run them.

@wallymathieu
Copy link
Member

@rzikm Hi! What kind of issues around running the tests do you have?

@gusty
Copy link
Member

gusty commented Jan 5, 2024

@rzikm That's fine, I also rely mostly in CIs run. You can keep adding commits, I will squash in the end.

Also, if you want to run them locally do it in Release mode, I don't think they will compile in DEBUG mode due to an old F# compiler bug.

PS: Looking at the test results, it seems to me that the Fable implementation you did is wrong.

@rzikm
Copy link
Contributor Author

rzikm commented Jan 5, 2024

@rzikm Hi! What kind of issues around running the tests do you have?

I was hitting the DEBUG compiler bug gusty mentioned.

I also got snagged because there is more than one WrappedListD type and I added a function to only one of them. Maybe there is a room for internal cleanup?

@gusty
Copy link
Member

gusty commented Jan 5, 2024

I also got snagged because there is more than one WrappedListD type and I added a function to only one of them. Maybe there is a room for internal cleanup?

AFAIK There's only one, which is in General.fs line 70

The main purpose of these types was to test overloads paths, but @wallymathieu recently added a better mechanism for it in #557

Still these wrapper types are sometimes useful to perform some tests as they are user-defined classes.

Independently from that, cleanups are always welcome, but then you have to consider either doing them in a separate PR or in a separate commit and then squash all the other unrelated commits.

My advice is a separate commit so you can draft it first and we can give early feedback.

BTW: when doing a separate PR or opening a new issue please mention me because somehow I never managed to get notifications for them until I participate.

@rzikm
Copy link
Contributor Author

rzikm commented Jan 5, 2024

AFAIK There's only one, which is in General.fs line 70

The other one is in

type WrappedListD<'s> = WrappedListD of 's list with

@gusty
Copy link
Member

gusty commented Jan 5, 2024

Oh, but those are Fable tests.
Fable tests are independent, they have their own set of wrappers.

We can unify some stuff, but sometimes it gets complicated.

@gusty
Copy link
Member

gusty commented Jan 6, 2024

This seems to be ready to merge. The failure in appveyor is a transient one.

Copy link
Member

@gusty gusty 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 to me. Just 2 nits.
In fact, there's room for optimizations by using ListCollector and possibly ArrayCollector, but to be fair, the same would apply to findSliceIndex, so it's better to leave both for a separate PR.

Thanks for this contribution !

src/FSharpPlus/Internals.fs Outdated Show resolved Hide resolved
src/FSharpPlus/Internals.fs Outdated Show resolved Hide resolved
Co-authored-by: Gustavo Leon <1261319+gusty@users.noreply.github.com>
@gusty gusty merged commit df90803 into fsprojects:master Jan 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal] String.(try)FindLastSliceIndex
3 participants