Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connect to each channel in PublicResponses automatically. #17

Merged
merged 2 commits into from
Aug 6, 2018

Conversation

liamphmurphy
Copy link
Contributor

So this adds the functionality desired from issue #14.

I am only including PublicResponses in this pull for 2 reasons:

  • See if others think this is a reasonable solution.
  • I wasn't sure if there would be a realistic possibility of there being channels in PublicResponses that wouldn't be in dmRepsonses and ephResponses. If this can occur, I'll work on a solution to this without re-joining the same channels multiple times.

My testing shows that the bot joins every public channel in config.json, and errors out when a channel is gone / private without closing the program.

Feedback would be great, thanks!

@heyitsols
Copy link
Contributor

Looks good @murnux 👍

From what we have seen, most channels have one of either a public, direct, or ephemeral response, so personally I think the ideal behaviour would be for it to check all three lists, unique them, and then iterate through joining in the way you have implemented

@lucymhdavies
Copy link
Contributor

lucymhdavies commented Aug 6, 2018

Thanks for the PR, @murnux 😄

I agree with @muggahtee on this. I think it should include all the lists, and attempt to join channels from all three types of responses.

In our configuration for example, most channels only have one type of response.

I'd also be inclined to move the "join every channel" logic to just before the main loop. It looks like it would be constantly trying to join channels at the moment. Given that the config doesn't change while the app is running, which seems unnecessary.

Only other thing I'd say is that the logging should be consistent with the rest of the code.
We use https://github.com/sirupsen/logrus

So, the equivalent of fmt.Println("Error joining public channel: %s", err) would be log.Errorf("Error joining public channel: %s", err) 🙂

@liamphmurphy
Copy link
Contributor Author

liamphmurphy commented Aug 6, 2018

Thanks for the input! I've pushed the new implementation. In case it isn't clear, here's what I've changed.

  • I believe that a different struct for public, dm, eph responses was not necessary because the data types are the exact same. So now they are all under slackResponse. It makes using the functions I created easier. (Of course, if you wanted to leave those there for a future purpose, let's change it back.)
  • I added two new functions: getChannelList iterates each type of response type, and creates a slice with all of them, including the duplicates. removeDuplicates takes the slice created by getChannelList and removes duplicates.
  • Before the main loop starts, this last slice is iterated and joins channels.

More input is appreciated, I actually went to bed last night thinking: "Wait, I iterated over the responses in the main loop, didn't I?" Haha.

EphResponses []ephResponse `json:"ephresponses"`
PublicResponses []slackResponse `json:"responses"`
DmResponses []slackResponse `json:"dmresponses"`
EphResponses []slackResponse `json:"ephresponses"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Well spotted. Yeah, no need for these to be separate types if they're all the same. :)

@lucymhdavies lucymhdavies merged commit d4b0d53 into skybet:master Aug 6, 2018
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.

3 participants