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

add splitOn #182

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add splitOn #182

wants to merge 2 commits into from

Conversation

ggPeti
Copy link

@ggPeti ggPeti commented Aug 13, 2024

I'm rude. My code is good. Accept the PR for the code or reject it for the person's rudeness. The choice is yours.

@Janiczek
Copy link
Contributor

For reviewers: the intuitive sense as to why this doesn't need any List.reverse nor ++ is that instead of tail-optimization it explodes the calculation into many nested function calls.

@ggPeti I think it could be useful to mention in the doc comment for the function that this keeps the delimiters

@miniBill
Copy link

Doesn't this cause a stack overflow for very long lists?

@ggPeti
Copy link
Author

ggPeti commented Aug 13, 2024

@Janiczek thanks, isn't "starting with each element" clear in communicating that? that was my intention

@miniBill yes it does. It's a purposefully stack-utilizing recursive function.

@miniBill
Copy link

I think one of the requisites we have for -extra functions is that they are stack safe.

By the way, the PR should probably be against elmcraft/core-extra.

@ggPeti
Copy link
Author

ggPeti commented Aug 13, 2024

I refuse to submit PRs to elmcraft/core-extra. And requiring stack safety indiscriminately is just silly and no such guarantee is documented in this library.

@miniBill
Copy link

Acknowledged

@ggPeti
Copy link
Author

ggPeti commented Aug 20, 2024

Hi @Chadtech could you please approve the checks?

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.

3 participants