Skip to content

Conversation

@joncamp
Copy link

@joncamp joncamp commented Mar 16, 2015

Fix channel.mark RequestPath bug
Add is_channel to Channel object
Update Response object to return reason-code as string
Add filetype to UploadFile

Fix channel.mark RequestPath bug
Add is_channel to Channel object
Update Response object to return reason-code as string
Add filetype to UploadFile
@Inumedia
Copy link
Owner

So briefly looking at this, if I recall correctly, channels and groups are interchangeable and there's no downside to grouping them together as a single object. Has this changed since I first implemented channel support?

@joncamp
Copy link
Author

joncamp commented Mar 16, 2015

Looking at a couple of the various methods, it depends. Some return back a group object such as https://api.slack.com/methods/groups.create, https://api.slack.com/methods/groups.createChild, https://api.slack.com/methods/groups.invite. Others return a channel object, such as https://api.slack.com/methods/groups.rename, return back a channel object.

@Inumedia
Copy link
Owner

The two other things that stand out are RPCMessages/GroupResponse.cs seemingly never being used, and account_inactive field being removed.

Re: Channel vs Group object, it looks like they resemble each other a lot, maybe some inheritance. I'd rather reduce the number of duplicate fields where possible for simplicity or at least having them easily interchangeable.

Re: groups.rename, it looks like it returns a partial view of the groups object since it contains the is_group = true field. This might fall back to the interchangeable thing where groups and channels are very similar, if not interchangeable.

Could you look into making channels / groups interchangeable? If not I'll look into it later this week. Once it's done or it's determined to not be a good idea to make them interchangeable, I'll pull it in.

@Inumedia
Copy link
Owner

See branch: https://github.com/Inumedia/SlackAPI/tree/joncamp-groups

If this looks alright to you and checks out, I'll pull it in.

@joncamp
Copy link
Author

joncamp commented Mar 18, 2015

Yeah, seems reasonable - thanks!

@Inumedia Inumedia merged commit 89524f0 into Inumedia:master Mar 18, 2015
@Inumedia
Copy link
Owner

Not a problem, and thanks for the help. If you have any other suggestions feel free to put them here or email me. :)

@joncamp joncamp deleted the groups branch March 18, 2015 21:08
Inumedia pushed a commit that referenced this pull request May 25, 2016
gpailler pushed a commit that referenced this pull request Nov 1, 2018
Inumedia pushed a commit that referenced this pull request Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants