slack room topic updates failing.

[balajijegan]

Discovered today that slack room topic updates were failing silently. I could see that the calls were being made to slack api but nothing in the logs.

further introspection, led me to this attribute in slackminion/slack/room/attributes.py

class SlackRoomTopic(SlackRoomAttribute):
    """Extension of SlackRoomAttribute, makes API call to set topic when __set__ is called"""
    def __set__(self, instance, value):
        prev_value = self.data
        super(SlackRoomTopic, self).__set__(instance, value)
        if prev_value is not None and prev_value != value:
            instance.set_topic(value)

My grokking of python docs says that __set__ descriptor should not return anything. Since topic updates can fail, I am thinking of removing this attribute. Users of this method should call the set topic explicitly.

@arcticfoxnv any issues with this approach? Or there other ways this could be handled?

based on response, I can send in a patch.

[arcticfoxnv]

I think I had written it this way to make use of magic in other plugins. I don't believe any of the core plugins use it, so changing it should be fine. However, any plugins that were written to make use of it will fail until they are rewritten.

[balajijegan]

I am going to do a quick scan of all plugins doing this both on GitHub and internally at Pinterest.

I will submit this in a way to avoid any potential issues.

Thanks for the response

On Wed, Sep 5, 2018, 5:28 PM Nick [email protected] wrote:

I think I had written it this way to make use of magic in other plugins. I don't believe any of the core plugins use it, so changing it should be fine. However, any plugins that were written to make use of it will fail until they are rewritten.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/arcticfoxnv/slackminion/issues/9#issuecomment-418923812, or mute the thread https://github.com/notifications/unsubscribe-auth/AANVbe2VJaXQcsg2uUQ3HrC1eHrTLQCsks5uYGwfgaJpZM4WaMZV .