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

intersperse for highland integration #1043

Merged
merged 1 commit into from
May 5, 2015

Conversation

buzzdecafe
Copy link
Member

No description provided.

if (idx === length - 1) {
out = out.concat(list[idx]);
} else {
out = out.concat(list[idx], separator);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use push instead of concat

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we decide to use xs[xs.length] = x? See #891.

I'd be happy for us to reverse that decision, but were we to do so it would be orthogonal to this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Push is just better in this case David
On Apr 21, 2015 9:51 PM, "David Chambers" notifications@github.com wrote:

In src/intersperse.js
#1043 (comment):

  • * @param {*} separator The element to add to the list.
  • * @param {Array} list The list to be interposed.
  • * @return {Array} The new list.
  • * @example
  • * R.interpserse('n', ['ba', 'a', 'a']); //=> ['ba', 'n', 'a', 'n', 'a']
  • */
    +module.exports = _curry2(_checkForMethod('intersperse', function intersperse(separator, list) {
  • var out = [];
  • var idx = -1;
  • var length = list.length;
  • while (++idx < length) {
  • if (idx === length - 1) {
  •  out = out.concat(list[idx]);
    
  • } else {
  •  out = out.concat(list[idx], separator);
    

Didn't we decide to use xs[xs.length] = x? See #891
#891.

I'd be happy for us to reverse that decision, but were we to do so it
would be orthogonal to this change.


Reply to this email directly or view it on GitHub
https://github.com/ramda/ramda/pull/1043/files#r28838931.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Push is just better in this case

Because we're pushing two values? Fair enough.

I'd like to use push in all cases, for the record. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So would I. I'm really not sure how we ended up in our current state.

@CrossEye
Copy link
Member

Agree with @megawac on push/concat, but other than that, LGTM.

* @func
* @memberOf R
* @category List
* @sig * -> [a] -> [a]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the return value has type [a] the separator must have type a.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm. really, the input array could be [*] and the return could be [*], so the whole sig probably should be * -> [*] -> [*].

@CrossEye
Copy link
Member

Do we describe these with the most generic signature that fits or the expected one? Clearly a -> [a] -> [a] is nicer and captures the likely use of the function. But there is nothing to prevent, say,

intersperse (new Date, ['a', 1, /\d+/, {}, []]):

I don't know that we have any choice but to document the most generic version, but it certainly doesn't make me happy.

@davidchambers
Copy link
Member

I prefer a -> [a] -> [a] here. I prefer to capture the intended use of the function. If we're completely honest we can end up with types such as * -> * -> * which aren't particularly helpful.

@buzzdecafe
Copy link
Member Author

I prefer a -> [a] -> [a] here.

i'm fine with that too. will revise when able

@CrossEye
Copy link
Member

Go ahead, twist my arm.

Ok, ok, I'm convinced!

* @return {Array} The new list.
* @example
*
* R.interpserse('n', ['ba', 'a', 'a']); //=> ['ba', 'n', 'a', 'n', 'a']

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo, intersperse

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch, thanks

@buzzdecafe
Copy link
Member Author

ping

👶 ➡️ 👴

@davidchambers
Copy link
Member

🌳

davidchambers added a commit that referenced this pull request May 5, 2015
intersperse for highland integration
@davidchambers davidchambers merged commit b6c7fc9 into ramda:master May 5, 2015
@buzzdecafe buzzdecafe deleted the intersperse-hl branch May 12, 2015 20:37
@anton-rudeshko
Copy link
Contributor

Great and long awaited addition, thanks! I implemented same extension in lodash, but it wasn't accepted.

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

6 participants