Skip to content

Conversation

@evnsio
Copy link
Contributor

@evnsio evnsio commented Sep 10, 2019

This is mostly for use in development. My instance tried to create comms channel #inc-101, but that channel existed already and had been archived, which the app didn't handle.

Copy link
Contributor

@milesbxf milesbxf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've added linting and formatting to the PR check - you should be able to do a pip install black isort and run ./autoformat from the root (it's worth setting up support in your editor for these).

Linting was how I caught the duplicate function issue, so definitely useful already!


raise SlackError(f"Channel '{name}' not found")

def unarchive_channel(self, channel_id):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already an unarchive_channel function on line 143, which will override this

@ChrisAnn ChrisAnn force-pushed the auto_unarchive branch 4 times, most recently from 645f1a6 to 9e530ff Compare October 10, 2019 13:08
@ChrisAnn ChrisAnn force-pushed the auto_unarchive branch 2 times, most recently from 601f0eb to 7fbacef Compare October 10, 2019 15:30
This handles the case of re-using an existing comms channel
that may have previously been archived as we won't automatically
join it on re-openning.
@ChrisAnn ChrisAnn force-pushed the auto_unarchive branch 2 times, most recently from ca0f62d to 21cbde2 Compare October 10, 2019 15:46
logger.info(f"Getting channel ID for {name}")
next_cursor = None

while next_cursor != "":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
while next_cursor != "":
while next_cursor:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(empty string is False-y)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this includes a whitespace change.

Copy link
Contributor

@mattrco mattrco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good (we could also clean up the next_cursor assignment using response.get(...)) but that doesn't need to be done here 👍

@ChrisAnn ChrisAnn merged commit cd41ec1 into master Oct 11, 2019
@ChrisAnn ChrisAnn deleted the auto_unarchive branch October 11, 2019 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants