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

Minor placeholder cleanup. #31

Merged
merged 2 commits into from
Oct 22, 2015
Merged

Minor placeholder cleanup. #31

merged 2 commits into from
Oct 22, 2015

Conversation

rtm
Copy link
Contributor

@rtm rtm commented Oct 21, 2015

No need to use a symbol here--just use a unique object.
Saves us a dependency and a file.

This commit has no effect on user-visible functionality.
Unit tests pass.

No need to use a symbol here--just use a unique object.
Saves us a dependency and a file.

This commit has no effect on user-visible functionality.
Unit tests pass.
@stoeffel
Copy link
Collaborator

We discussed this in #25 and @tomekwi had a really good point on this #25 (comment)

@rtm
Copy link
Contributor Author

rtm commented Oct 21, 2015

@stoeffel Not sure which comment you are referring to. If it is the one about Symbol being better than string, of course it is, but it's not necessary when you can just use {}.

Please try out the PR if you have time. It works perfectly in every regard, without using symbols.

@stoeffel
Copy link
Collaborator

@rtm Will check it out, as soon as possible.
Thx for your contributions anyways 😃 It's always good to have stuff challenged.

@davidchase
Copy link
Member

If we were to get rid of the symbol polyfill we should at least keep the Symbol naming convention such doing something like @@currythis just my $0.02

let curryPlaceholder = '@@currythis';
if(typeof Symbol === 'function' && typeof Symbol.for === 'function') {
    curryPlaceholder = Symbol.for('currythis');
}

@rtm
Copy link
Contributor Author

rtm commented Oct 21, 2015

@davidchase Why? This PR uses a unique empty object as a placeholder. This is fast and easy to compare for strict equality, and requires neither symbols nor strings.

@stoeffel
Copy link
Collaborator

@rtm agreed. I would prefer an object over a string. I actually don't see a reason why we couldn't merge this. But I will have a night sleep over it and decide tomorrow.

@davidchase
Copy link
Member

@rtm simple... better interop between libraries read here

@rtm
Copy link
Contributor Author

rtm commented Oct 21, 2015

@davidchase Well, interop will work only if all libraries adopt the same convention, which seems unlikely.

Anyway, thanks for referring to that thread. As far as I can tell, it seems to be discussing two different issues:

  1. If I mix-and-match two different currying libraries, then I might get confused with their different placeholders. Well, yes. I brought that confusion on myself. But I can just use each currying libraries correct value of its placeholder.
  2. If I bring in curry-this myself, and then I have some other dependency X which itself brings in curry-this as dependency, the placeholders for the two versions of the library in my dependency tree will be different. But this seems like bikeshedding. X is presumably using curry-this for its own purposes, and will use the placeholder defined by its version of curry-this to do its business, and I will use curry-this for my purposes, and will use the placeholder defined by my version of curry-this to do my business. All is well.

@tomek-he-him
Copy link
Member

@davidchase I agree with @rtm for that matter.

As well as that – when one module defines const _ = Symbol('A') and another defines defines const _ = Symbol('A') then the two _s are different things anyway. So our discussion seems irrelevant. (Correct me if I’m wrong.)

@rtm
Copy link
Contributor Author

rtm commented Oct 22, 2015

@stoeffel PR updated with new placeholder value {placeholder: 'curry-this'}.

@stoeffel
Copy link
Collaborator

Awesome thx!

stoeffel added a commit that referenced this pull request Oct 22, 2015
@stoeffel stoeffel merged commit 43eac8d into thisables:master Oct 22, 2015
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.

4 participants