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] Make comparable label-less #2565

Merged
merged 1 commit into from Mar 6, 2018
Merged

Conversation

chenglou
Copy link
Member

@chenglou chenglou commented Mar 3, 2018

It's a single arg, so reduces the burden of using that pattern a little.

I'll update the docs after this

It's a single arg, so reduces the burden of using that pattern a little.

I'll update the docs after this
@chenglou
Copy link
Member Author

chenglou commented Mar 3, 2018

cc @cristianoc

@bobzhang
Copy link
Member

bobzhang commented Mar 3, 2018

I thought about this. The thing is that when creating hashtbl, you need to pass ~hash ~eq, to make it more consistent, I chose to keep it.

@chenglou
Copy link
Member Author

chenglou commented Mar 4, 2018

It's not inconsistent not to have a label, especially when the call's name is comparable though. I'm trying to address the feedback and make this piece of boilerplate a bit easier to swallow.

module IntCmp = (
  val Belt.Id.comparable((x: int, y) => Pervasives.compare(x, y))
);

let s = make(~id=(module IntCmp));

@rickyvetter
Copy link
Contributor

Yeah I think ~cmp is redundant with comparable and is hard to understand when seen/used by itself (component, compute both could be shortened to cmp). Since neither of those two outcomes are desirable I'd prefer to remove as well.

@chenglou chenglou merged commit 1c27b71 into rescript-lang:master Mar 6, 2018
@chenglou
Copy link
Member Author

chenglou commented Mar 6, 2018

Discussed offline

@chenglou chenglou deleted the comp branch March 6, 2018 08:23
chenglou added a commit to chenglou/rescript-compiler that referenced this pull request Mar 6, 2018
chenglou added a commit to chenglou/rescript-compiler that referenced this pull request Mar 6, 2018
chenglou added a commit that referenced this pull request Mar 6, 2018
chenglou added a commit to chenglou/rescript-compiler that referenced this pull request Mar 9, 2018
See rescript-lang#2565 which removed it. rescript-lang#2589 deprecates comparable, so we can just
put the label back to avoid unnecessary churn the next release
chenglou added a commit that referenced this pull request Mar 9, 2018
See #2565 which removed it. #2589 deprecates comparable, so we can just
put the label back to avoid unnecessary churn the next release
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.

None yet

3 participants