-
-
Notifications
You must be signed in to change notification settings - Fork 59
Conversation
1a1b99e
to
4ac453c
Compare
4ac453c
to
4a17513
Compare
&& !!user.userId | ||
) | ||
.map((user) => user.screenName) | ||
.value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the thankList
iteration to the end of the method and no need to unwrap it before. This ensures best lazy evaluation performance.
Any progress? As per ☝️ This Comment in HelpContributors
|
@SaintPeter that is addressed by @noncentz in https://github.com/FreeCodeCamp/camperbot/pull/78 Just need to remove the line linked here and then that can be merged. |
I'm really confused what is going on in this thread. @abhisekp Is this ready? I see you've written a bunch of critiques on your own PR so I'm not really sure what the status of this is. |
@ltegman it is not ready yet.. I've to implement the suggestions. Bit busy these days... Will correct them ASAP and ping you. 👍 |
- Use lodash - Discard non-existing users and self and thank only existing ones (which also fixes thanking @/all as a bonus) E.g. if a user doesn't exist in gitter, then there will be no API calls to thank that user.
- fixed some messages - formatted source a bit
4a17513
to
04b9b6e
Compare
@abhisekp I know you are busy, but this would be great to have this soon. ;) |
Closing as stale. Please reopen when this is ready to be merged. |
Improved
- **Using `lodash` for better performance due to lazy evaluation** - **Discards non-existing users and self while thanking and thank only existing ones** (which also fixes thanking `@/all` as a bonus) i.e. if a user doesn't exist in gitter, then there will be no API calls to thank that user.thanks
command implementationFixes #76
Fixes dcsan/gitterbot#170