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

Change pluck to be more prop-like #293

Merged
merged 1 commit into from
Nov 27, 2016

Conversation

svozza
Copy link
Member

@svozza svozza commented Nov 25, 2016

Closes #291

eq(S.pluck(Array, 'x', [{x: vm.runInNewContext('[0]')}]), [S.Just([0])]);
eq(S.pluck(vm.runInNewContext('Array'), 'x', [{x: [0]}]), [S.Just([0])]);
eq(S.pluck('x', [{x: vm.runInNewContext('[0]')}]), [[0]]);
eq(S.pluck(vm.runInNewContext("'x'"), [{x: [0]}]), [[0]]);
Copy link
Member

Choose a reason for hiding this comment

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

These vm test are no longer relevant. Let's remove them.

//. Takes a property name and an array of objects and returns an array in
//. which each element is the value of the specified property of the
//. corresponding object. If for some reason the object lacks the specified
//. property, a type error is thrown.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to leverage the documentation for existing functions:

Combines [`map`][R.map] and [`prop`](#prop). `pluck(k, xs)` is equivalent
to `map(prop(k), xs)`.


[R.map]: http://ramdajs.com/docs/#map

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and let's remember to change R.map to S.map when we release 0.12.

return xs.map(function(obj, idx) {
if (key in Object(obj)) return obj[key];
throw new TypeError('‘pluck’ expected object at index ' + idx + ' to ' +
'have a property named ‘' + key + '’; ' +
Copy link
Member

Choose a reason for hiding this comment

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

Let's move ' to ' down one line for more pleasing line wrapping (saving one concatenation operation in the process).

Copy link
Member Author

Choose a reason for hiding this comment

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

function pluck(typeRep, key, xs) {
return xs.map(function(x) { return get(typeRep, key, x); });
function pluck(key, xs) {
return xs.map(function(obj, idx) {
Copy link
Member

Choose a reason for hiding this comment

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

I like to see an element of xs referred to as x.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidchambers
Copy link
Member

refactor pluck to be more prop-like

Strictly speaking this is not refactoring since we're changing the function's behaviour. Would you mind updating the commit message and pull request title?

@svozza svozza changed the title refactor pluck to be more prop-like Change pluck to be more prop-like Nov 26, 2016
@@ -3500,6 +3500,7 @@
//. [R.equals]: http://ramdajs.com/docs/#equals
//. [Ramda]: http://ramdajs.com/
//. [RegexFlags]: https://github.com/sanctuary-js/sanctuary-def#regexflags
//. [R.map]: http://ramdajs.com/docs/#map
Copy link
Member

Choose a reason for hiding this comment

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

Could you insert this line after [R.equals] instead? I like to type vip:sort u in Vim to sort this "paragraph", so keeping this list sorted will avoid noise in future diffs.

Copy link
Member Author

Choose a reason for hiding this comment

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

//.
//. See also [`get`](#get).
//. Combines [`map`][R.map] and [`prop`](#prop). `pluck(k, xs)` is equivalent
//. to `map(prop(k), xs)`.
Copy link
Member

Choose a reason for hiding this comment

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

Shall we add See also [`pluck`](#pluck). to the prop docstring?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good idea.

@davidchambers
Copy link
Member

🌳

@davidchambers davidchambers merged commit edfa59e into sanctuary-js:master Nov 27, 2016
@svozza
Copy link
Member Author

svozza commented Nov 27, 2016

It's good to be back. ;)

@svozza svozza deleted the 291-pluck branch November 27, 2016 14:06
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.

2 participants