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 List.find #303

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@rtfeldman
Member

rtfeldman commented Jul 21, 2015

This adds List.find, a useful function I've been using in a personal Util.elm file, but which seems like a good candidate for inclusion in core given that it's standard in several languages, for example:

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 21, 2015

This should proxy to Native right? I've been wondering whether there's criteria for Core inclusion.

texastoland commented Jul 21, 2015

This should proxy to Native right? I've been wondering whether there's criteria for Core inclusion.

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Jul 21, 2015

Member

Would proxying to Native improve this though? With the impending self-TCO this compiles to a while loop anyway, yeah?

Member

rtfeldman commented Jul 21, 2015

Would proxying to Native improve this though? With the impending self-TCO this compiles to a while loop anyway, yeah?

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 21, 2015

Native JS uses C++ cheats. Based on O(1) optimizations V8 performs on property access it's reasonable to imagine find() using a reverse cache lookup on object ID. In general if there's a native function we should always proxy.

texastoland commented Jul 21, 2015

Native JS uses C++ cheats. Based on O(1) optimizations V8 performs on property access it's reasonable to imagine find() using a reverse cache lookup on object ID. In general if there's a native function we should always proxy.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 21, 2015

PS I agree it should be included I just wish we had a criteria for completeness and rejection.

texastoland commented Jul 21, 2015

PS I agree it should be included I just wish we had a criteria for completeness and rejection.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 21, 2015

Member

For now the recommended way of extending the standard libraries is in a package, like elm-list-extra or elm-random-extra. There can be lots of experimentation with APIs there that does not block on me or the release cycle of elm-lang/core. At some point, we will go through and try to merge in all the best things about the *-extra world in a coherent way.

I suspect @rehno-lindeque would accept this function in elm-list-extra, try a PR there for now.

Member

evancz commented Jul 21, 2015

For now the recommended way of extending the standard libraries is in a package, like elm-list-extra or elm-random-extra. There can be lots of experimentation with APIs there that does not block on me or the release cycle of elm-lang/core. At some point, we will go through and try to merge in all the best things about the *-extra world in a coherent way.

I suspect @rehno-lindeque would accept this function in elm-list-extra, try a PR there for now.

@evancz evancz closed this Jul 21, 2015

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 21, 2015

Thanks for the clarification @evancz makes sense to me 👍

texastoland commented Jul 21, 2015

Thanks for the clarification @evancz makes sense to me 👍

@rtfeldman rtfeldman referenced this pull request Jul 21, 2015

Merged

Add Extra.find #9

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Jul 21, 2015

Member

Fair enough! Moved the PR to elm-list-extra.

Member

rtfeldman commented Jul 21, 2015

Fair enough! Moved the PR to elm-list-extra.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment