Add filter and find by account function to contributors #42

2018-04-23T14:30:13Z

bumi 评论于

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

(从 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 (从 github.com 迁移) 请求变更 2018-04-25 21:53:41 +00:00

fsmanuel (从 github.com 迁移) 留下了一条评论

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 评论于

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

(从 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 评论于

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

(从 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 评论于

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

(从 github.com 迁移)

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

raucao 评论于

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

(从 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 评论于

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

(从 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 评论于

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

(从 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 评论于

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

(从 github.com 迁移)

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

raucao 评论于

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

(从 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 评论于

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

(从 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 评论于

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

(从 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 评论于

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

(从 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 评论于

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

(从 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 评论于

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

(从 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 评论于

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

(从 github.com 迁移)

what do you think of this now?

raucao (从 github.com 迁移) 于 2018-04-26 12:55:17 +00:00 批准此合并请求

raucao (从 github.com 迁移) 留下了一条评论

Nice! 👏

Nice! :clap:

fsmanuel (从 github.com 迁移) 评审于 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 (从 github.com 迁移) 评论于

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

That's a master piece of functional programming 😀

fsmanuel (从 github.com 迁移) 评审于 2018-04-26 13:26:19 +00:00

lib/contracts/contributor.js

fsmanuel (从 github.com 迁移) 评论于

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 (从 github.com 迁移) 评审于 2018-04-26 13:36:36 +00:00

lib/contracts/contributor.js

bumi (从 github.com 迁移) 评论于

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

oh, reads nice. thanks.

fsmanuel (从 github.com 迁移) 于 2018-04-26 14:26:47 +00:00 批准此合并请求

fsmanuel (从 github.com 迁移) 留下了一条评论

♥️

登录并参与到对话中。

1 名参与者

通知

到期时间

未设置到期时间。

依赖工单

没有设置依赖项。

参考：kredits/contracts#42