Skip to content
This repository has been archived by the owner on Feb 15, 2022. It is now read-only.

Added send/buy orders notifications using Slack's Incoming WebHooks API. #583

Closed
wants to merge 1 commit into from
Closed

Conversation

flocca
Copy link

@flocca flocca commented Oct 3, 2017

No description provided.

@evseevnn-zz
Copy link
Contributor

@flocca i think bad idea implement it inside engine.
i think that should be like module. Im sure what many ppl wanna write notification module for this project and we need prepare possibility for that.

Please try refactor your code and make it better.

@flocca
Copy link
Author

flocca commented Oct 4, 2017

Ok. I can code this like a zenbot extension, maybe under extensions/notifications/, a place where we can put all the notifications modules (slack.js, email.js, etc.). What do you think about all this @evseevnn?

@evseevnn-zz
Copy link
Contributor

@flocca Yes! That good idea! You will make a great contribution to the development of this library if you do this.

@tuxitor
Copy link
Contributor

tuxitor commented Oct 4, 2017

@flocca
I agree with @evseevnn in this matter. XMPP has already been added to "lib/engine.js". To continue adding messaging options this way only leads to more code pollution. I recommend reading this thread:
#333,
and make note of the comments by @DeviaVir and @tuxitor. Then take a look at the "zentalk" branch on this fork where a more generiic approach is used for messaging: https://github.com/tuxitor/zenbot/tree/zentalk
The documentation should be informative: https://github.com/tuxitor/zenbot/blob/zentalk/docs/Zentalk.md
The "zenxmpp" program should make a good base for implementing any messaging client. Here is an example:
zenxmpp

@DeviaVir
Copy link
Owner

DeviaVir commented Oct 5, 2017

I think I'll close this PR for now, the changes are good, but since you have the intention of rewriting it doesn't make too much sense to keep it around.

@DeviaVir DeviaVir closed this Oct 5, 2017
@flocca
Copy link
Author

flocca commented Oct 5, 2017

Thanks @DeviaVir, I agree.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants