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

[Belt] Add List.filter/WithIndex with deprecation warning and point to List.keep/WithIndex #3072

Merged
merged 4 commits into from
Oct 9, 2018

Conversation

kutyel
Copy link
Contributor

@kutyel kutyel commented Oct 2, 2018

Hi, I found this pretty confusing since the native List OCaml does contain a filter method, I guess every js dev would expect Belt.List to have exactly that naming as well.

This also came up during @ryyppy workshop in Alicante and I noticed some people were confused as well.

For now can we consider to have an alias to that fns? @IwanKaramazow also mentioned to me something about a deprecation flag in OCaml but I have literally no idea how to do that 😄

@chenglou
Copy link
Member

chenglou commented Oct 5, 2018

We should have a filter with an unusable signature, plus a deprecation warning that redirects you to keep. You'd add a [@@ocaml.deprecated "foo"] before the declaration in the mli file.

cc @bobzhang

Edit: or maybe just the deprecation and an alias

@kutyel
Copy link
Contributor Author

kutyel commented Oct 5, 2018

@chenglou hi! Do you mean to mark as deprecated the keep function in favor of filter or the other way round? 😄

@chenglou
Copy link
Member

chenglou commented Oct 5, 2018

The other way around, so exactly what you did, but with a [@@deprecated "message here"] in the mli file. Also, no need to test it.
We should do the same for other modules too.

@kutyel
Copy link
Contributor Author

kutyel commented Oct 7, 2018

I think this is ready 🚀 @chenglou @bobzhang 😉

@kutyel kutyel changed the title [Belt][Proposal] Rename List.keep to List.filter [Belt] Add List.filter/WithIndex with deprecation warning and point to List.keep/WithIndex Oct 7, 2018
@bobzhang bobzhang merged commit 1f47f16 into rescript-lang:master Oct 9, 2018
@bobzhang
Copy link
Member

For the record, the attribute should be put behind the val declaration (not in front of)

@chenglou
Copy link
Member

Sorry that's my bad, I was thinking in Reason syntax. @kutyel could you make the change please? And I imagine that when you use filter there's no warning currently?

@bobzhang
Copy link
Member

@chenglou I already did, no worries

@kutyel kutyel deleted the feat/keep-to-filter branch October 14, 2018 16:12
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