Add filter and find by account function to contributors #42

2018-04-23T14:30:13Z

bumi commented

2018-04-23 14:30:13 +00:00

(Migrated from github.com)

This allows to filter and find contributors by the account entries.

For example:


Contributor.filterByAccount({site: 'github.com'}); // returns an array of all contributors with github account
Contributor.filterByAccount({site: 'github.com', username: 'bumi'}); // returns an array with bumi as single entry

Contributor.findByAccount({site: 'github.com', username: 'bumi'}); // returns the matching contributor 
Contributor.findByAccount({site: 'github.com'}); // returns the matching contributor - the first with a github.com account entry

This allows to filter and find contributors by the account entries. For example: ```js Contributor.filterByAccount({site: 'github.com'}); // returns an array of all contributors with github account Contributor.filterByAccount({site: 'github.com', username: 'bumi'}); // returns an array with bumi as single entry Contributor.findByAccount({site: 'github.com', username: 'bumi'}); // returns the matching contributor Contributor.findByAccount({site: 'github.com'}); // returns the matching contributor - the first with a github.com account entry ```

fsmanuel (Migrated from github.com) requested changes 2018-04-25 21:53:41 +00:00

fsmanuel (Migrated from github.com) left a comment

We should always return an array like the native filter does: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Array/filter

We should always return an array like the native `filter` does: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Array/filter

bumi commented

2018-04-26 00:26:45 +00:00

(Migrated from github.com)

how would we do that? because of the promise that is returned by all() wouldn't we always need to also return a promise?

how would we do that? because of the promise that is returned by `all()` wouldn't we always need to also return a promise?

raucao commented

2018-04-26 10:28:08 +00:00

(Migrated from github.com)

I think he meant the promise should be fulfilled with an array, if it's called filter instead of find, so it's not misleading.

I think he meant the promise should be fulfilled with an array, if it's called `filter` instead of `find`, so it's not misleading.

👍 1

bumi commented

2018-04-26 11:16:52 +00:00

(Migrated from github.com)

it is returning the filter, line 41... no?

raucao commented

2018-04-26 11:33:03 +00:00

(Migrated from github.com)

It looks like it's returning the result of a find, which would be the first matching object in the filtered array.

It looks like it's returning the result of a `find`, which would be the first matching object in the filtered array.

bumi commented

2018-04-26 11:39:37 +00:00

(Migrated from github.com)

the return value of the find is used in the filter callback. if the callback returns a truthy value the contributor is kept otherwise removed. the return value of filter is returned by the promise.

probably

return find(...) != undefined;

would be better instead of simply returning the return value of the find method.

the return value of the find is used in the filter callback. if the callback returns a truthy value the contributor is kept otherwise removed. the return value of filter is returned by the promise. probably ```js return find(...) != undefined; ``` would be better instead of simply returning the return value of the find method.

raucao commented

2018-04-26 11:55:24 +00:00

(Migrated from github.com)

Why not just have an explicit findBy that actually returns a single contributor? I think it's a common use case to look up one, so checking array length and calling [0] in the client would be a bit clunky anyway.

Why not just have an explicit `findBy` that actually returns a single contributor? I think it's a common use case to look up one, so checking array length and calling `[0]` in the client would be a bit clunky anyway.

bumi commented

2018-04-26 11:56:28 +00:00

(Migrated from github.com)

because this way you can filter and not only get one single contributor.

raucao commented

2018-04-26 11:56:57 +00:00

(Migrated from github.com)

Yes, but you can have both filterBy and findBy. Why only have one function to do it all?

Yes, but you can have both `filterBy` and `findBy`. Why only have one function to do it all?

bumi commented

2018-04-26 11:59:33 +00:00

(Migrated from github.com)

not sure if I understand what you mean.
I try to get contributors by their account entry. that's why I filter the contributors array and find in the accounts array for matching entries.

the function is only doing one thing.
a findBy.. would be a filterBy()[0] ?

not sure if I understand what you mean. I try to get contributors by their account entry. that's why I filter the contributors array and find in the accounts array for matching entries. the function is only doing one thing. a `findBy..` would be a `filterBy()[0]` ?

raucao commented

2018-04-26 12:08:00 +00:00

(Migrated from github.com)

Contributor.filterByAccount(...); // returns bumi

I think both @fsmanuel and I read this as "returns bumi's contributor object", but it actually meant "returns an array with bumi as the only entry". So if it returns the latter, as you explained, then the original comment about it was invalid and just a misunderstanding.

However, what I mean by "why not have a findBy" was: a normal use case in clients is to actually find a single contributor's data. Then, filtering for exact values, implicitly expecting a single-entry array, is a bit clunky.

(I think the "give me all users with GitHub accounts" doesn't currently appear in kredits-web or hubot-kredits, but the "give me the data for @bumi on GitHub" does.)

> Contributor.filterByAccount(...); // returns bumi I think both @fsmanuel and I read this as "returns bumi's contributor object", but it actually meant "returns an array with bumi as the only entry". So if it returns the latter, as you explained, then the original comment about it was invalid and just a misunderstanding. However, what I mean by "why not have a findBy" was: a normal use case in clients is to actually find a single contributor's data. Then, filtering for exact values, implicitly expecting a single-entry array, is a bit clunky. (I think the "give me all users with GitHub accounts" doesn't currently appear in kredits-web or hubot-kredits, but the "give me the data for @bumi on GitHub" does.)

👍 1

fsmanuel commented

2018-04-26 12:23:01 +00:00

(Migrated from github.com)

@bumi It's just about the semantics filter vs find. If I see a filter in JS land I expect it to return an array. If I see a find I expect it to return a value or null.

I think your last example:

Contributor.filterByAccount({site: 'github.com', username: 'bumi'}); // returns bumi: { id: 123, ... }

// should be 
Contributor.findByAccount({site: 'github.com', username: 'bumi'}); // returns bumi: { id: 123, ... }

We can use a filterByAccount that returns an array in findByAccount that returns a value.

@bumi It's just about the semantics `filter` vs `find`. If I see a `filter` in JS land I expect it to return an array. If I see a `find` I expect it to return a value or `null`. I think your last example: ```js Contributor.filterByAccount({site: 'github.com', username: 'bumi'}); // returns bumi: { id: 123, ... } // should be Contributor.findByAccount({site: 'github.com', username: 'bumi'}); // returns bumi: { id: 123, ... } ``` We can use a `filterByAccount` that returns an array in `findByAccount` that returns a value.

bumi commented

2018-04-26 12:34:42 +00:00

(Migrated from github.com)

yes I see. the comment in the PR description is missleading. I've updated that.
The method that this PR adds always returns an array - it returns the return value of the native Array.filter function.

I tried to implement a reusable function to find a single contributor but thought this logic is more flexible and used filter instead of find. (line 41)

should I change it to findByAccount and use find instead of filter?
or add a findByAccont function that returns the fist value of the filterByAccount function?
or refactor it that it is possible to call filter or find?

(in the code right now, with this implementation, I'd use filterByAccount({})[0] )

yes I see. the comment in the PR description is missleading. I've updated that. The method that this PR adds always returns an array - it returns the return value of the native `Array.filter` function. I tried to implement a reusable function to find a single contributor but thought this logic is more flexible and used `filter` instead of `find`. (line 41) should I change it to `findByAccount` and use `find` instead of `filter`? or add a `findByAccont` function that returns the fist value of the `filterByAccount` function? or refactor it that it is possible to call filter or find? (in the code right now, with this implementation, I'd use `filterByAccount({})[0]` )

raucao commented

2018-04-26 12:38:01 +00:00

(Migrated from github.com)

I'd add a findBy, but actually only call find in that one, and call filter in the filter one. It's not clear that a filter function would include a find, thus restricting results to a single match. If we have both, then filter can filter, and find can find. :)

I'd add a `findBy`, but actually only call find in that one, and call `filter` in the filter one. It's not clear that a `filter` function would include a `find`, thus restricting results to a single match. If we have both, then filter can filter, and find can find. :)

bumi commented

2018-04-26 12:54:31 +00:00

(Migrated from github.com)

what do you think of this now?

raucao (Migrated from github.com) approved these changes 2018-04-26 12:55:17 +00:00

raucao (Migrated from github.com) left a comment

Nice! 👏

Nice! :clap:

fsmanuel (Migrated from github.com) reviewed 2018-04-26 13:22:15 +00:00

lib/contracts/contributor.js

						
				@ -37,0 +42,4 @@

				    return this._byAccount(search, 'find');

				  }

				  _byAccount(search, method = 'filter') {

fsmanuel (Migrated from github.com) commented

2018-04-26 13:22:15 +00:00

That's a master piece of functional programming 😀

fsmanuel (Migrated from github.com) reviewed 2018-04-26 13:26:19 +00:00

lib/contracts/contributor.js

fsmanuel (Migrated from github.com) commented

2018-04-26 13:26:19 +00:00

I would replace the reduce with an every Docs

searchEntries.every((item) => {
  const [ key, value ] = item;
  return account[key] === value;
});

I would replace the `reduce` with an `every` [Docs](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every) ```js searchEntries.every((item) => { const [ key, value ] = item; return account[key] === value; }); ```

bumi (Migrated from github.com) reviewed 2018-04-26 13:36:36 +00:00

lib/contracts/contributor.js

bumi (Migrated from github.com) commented

2018-04-26 13:36:36 +00:00

oh, reads nice. thanks.

fsmanuel (Migrated from github.com) approved these changes 2018-04-26 14:26:47 +00:00

fsmanuel (Migrated from github.com) left a comment

♥️

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kredits/contracts#42