search

huggingface / knockknock

🚪✊Knock Knock: Get notified when your training ends with only two additional lines of code
MIT License
2.78k stars 233 forks source link

Update slack notification formatting #34

Open MetcalfeTom opened 4 years ago

MetcalfeTom commented 4 years ago

Happy holidays! 🎅🏼 I thought the slack message formatting looked a little sad, so I updated it. I used the block kit builder to update the messages.

I also updated the slack_sender to use usernames instead of user_ids (perhaps I used this incorrectly, but posting the user_id never tagged the right person)

The reports look like this now:

Screenshot 2019-12-26 at 7 11 25 pm
VictorSanh commented 4 years ago

@MetcalfeTom I've allocated some time to review your PR this week. Something I am thinking about in the background is refactoring: a few of these platforms (Slack, Discord, MSFT Teams, etc.) use the same communication protocol (via web hook), so I'm wondering if I can do some refactoring here (it should be possible), and if so, whether this formatting update would be compatible with other platforms than Slack.

MetcalfeTom commented 4 years ago

Cool

Teams supports a similar formatting feature to blocks called cards. Discord does not have any kind of fancy formats. In all three cases the JSON payloads would still differ

VictorSanh commented 4 years ago

@MetcalfeTom it looks great! I added a few question/comments/concerns. For now, I'll suggest we push this formatting update only for Slack as it does not generalize to all platforms using webhook as a communication protocol.

MetcalfeTom commented 4 years ago

@VictorSanh Thanks for taking the time to review! 🙌

I followed up on your comments and added your suggestions. Not sure you'd construct the refactor between this, discord and teams, but following that PR I would gladly take a stab at updating the Teams format to use cards too 🚀