search

bonzanini / luigi-slack

Slack notifications for the Luigi workflow manager
MIT License
46 stars 13 forks source link

Include as_user argument to post messages as the authed user #3

Closed amarco90 closed 7 years ago

amarco90 commented 7 years ago

I thought it would be nice to be able to post the luigi notifications as a user/bot. Before, all notifications arrived in slackbot with the customized username. This PR allows setting as_user=True that makes the messages arrive in a user/bot chat, determined authomatically from the slack token.

(only tested with single users @username and not with channels)

amarco90 commented 7 years ago

@bonzanini do you think the PR makes sense? it is my first pull request in a public project, I would appreciate any kind of feedback :)

bonzanini commented 7 years ago

@amarco90 thanks for the contribution, it looks good but haven't found some time yet to test it properly, given the season :) realistically it will be merged soon

amarco90 commented 7 years ago

Thanks @bonzanini for the quick reply :) One issue that I thought about is setting the parameter as_user to something different than a boolean. If the variable is not a boolean I guess setting a default value or throwing an exception could be valid solutions. One option could be to include the line: as_user = as_user if isinstance(as_user, bool) else False in the constructor of SlackBot. What do you think?

bonzanini commented 7 years ago

@amarco90 I think I'm happy with the code as it is now, to let the Slack API deal with a non-bool argument. I might add a TypeError later (I prefer the exception rather than an implicit cast to a default value), but maybe it's not really critical