Add filter and find by account function to contributors #42
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "feature/filter-contributors-by-account"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This allows to filter and find contributors by the account entries.
For example:
We should always return an array like the native
filter
does: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Array/filterhow would we do that? because of the promise that is returned by
all()
wouldn't we always need to also return a promise?I think he meant the promise should be fulfilled with an array, if it's called
filter
instead offind
, so it's not misleading.it is returning the filter, line 41... no?
It looks like it's returning the result of a
find
, which would be the first matching object in the filtered array.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
would be better instead of simply returning the return value of the find method.
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.because this way you can filter and not only get one single contributor.
Yes, but you can have both
filterBy
andfindBy
. Why only have one function to do it all?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 afilterBy()[0]
?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.)
@bumi It's just about the semantics
filter
vsfind
. If I see afilter
in JS land I expect it to return an array. If I see afind
I expect it to return a value ornull
.I think your last example:
We can use a
filterByAccount
that returns an array infindByAccount
that returns a value.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 offind
. (line 41)should I change it to
findByAccount
and usefind
instead offilter
?or add a
findByAccont
function that returns the fist value of thefilterByAccount
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]
)I'd add a
findBy
, but actually only call find in that one, and callfilter
in the filter one. It's not clear that afilter
function would include afind
, thus restricting results to a single match. If we have both, then filter can filter, and find can find. :)what do you think of this now?
Nice! 👏
@ -37,0 +42,4 @@
return this._byAccount(search, 'find');
}
_byAccount(search, method = 'filter') {
That's a master piece of functional programming 😀
I would replace the
reduce
with anevery
Docsoh, reads nice. thanks.
♥️