Use conversations_list instead of deprecated channels_list #331
Conversation
|
I think you're also missing new mixins in https://github.com/slack-ruby/slack-ruby-client/tree/master/lib/slack/web/api/mixins for at least the conversations methods. |
| @@ -17,7 +17,7 @@ def channels_id(options = {}) | |||
| throw ArgumentError.new('Required arguments :channel missing') if name.nil? | |||
|
|
|||
| id_for(:channel, name, '#', :channels, 'channel_not_found') do | |||
| channels_list | |||
| conversations_list | |||
There was a problem hiding this comment.
When users call channels_ they get deprecations. I think we want to keep that. For those that call conversations_ we shouldn't have any deprecations. I don't think we should mix both. It's in id_for that we want to call the new(er) methods, therefore calling conversations_history won't show deprecations, which is the issue that was raised.
There was a problem hiding this comment.
I didn't quite follow this
It's in id_for that we want to call the new(er) methods
If I understood correctly, the conversations_-methods still use this same channels_id method, which would result in calling the deprecated channels_list method. Did you mean we should have some logic to determine whether channels_list or conversations_list is used, depending on whether it's a channels_ or conversations_ method call?
If so, I could make a copy of this method that uses conversations_list, and in the method templates use deprecated: true as a signal for whether to use this new method instead of channels_id. What do you think?
There was a problem hiding this comment.
I'll try again. I don't think we should rewrite channels_id, but deprecate it.
- introduce
Mixins::Conversations::Ids#conversations_idthat looks just like this method except change channels to conversations - deprecate
channels_idthe usual way with a warning - call
conversations_ideverywhere we callchannels_idexcept in deprecatedchannels_listmethod
This way we don't change this mix-in and fix any method that has not been deprecated.
There was a problem hiding this comment.
Sounds good, will go with that!
This reverts commit 42d4c4d.
|
@dblock there's a sizable set of changes that are incoming with the updated Aside: ActiveSupport was still required for the rake task, so I re-required it in the file. |
|
Hmm on second look the overall impact of this was the same as the previous implementation (swapping out |
|
As is this looks good to me (other than the failing build, rubocop), what am I missing? Channels mixin can be left alone, the template change makes sense as it defaults to the newer method now. |
| @@ -40,7 +40,7 @@ module Slack | |||
| <% if data['group'] == 'groups' && data['args']['channel'] && !data['args']['channel']['desc'].include?('Can be an encoded ID, or a name.') %> | |||
| options = options.merge(channel: groups_id(options)['group']['id']) if options[:channel] | |||
| <% elsif data['args']['channel'] && !data['args']['channel']['desc'].include?('Can be an encoded ID, or a name.') %> | |||
| options = options.merge(channel: channels_id(options)['channel']['id']) if options[:channel] | |||
| options = options.merge(channel: conversations_id(options)['channel']['id']) if options[:channel] | |||
There was a problem hiding this comment.
This is perfect, fixes the regression in all cases.
It seemed like the Channels mixin becomes essentially useless, as it's one and only method ( |
I think someone may be using it directly so we should keep it. We can delete it when the deprecated API is removed I think. Happy to merge on 🍏 |
|
Not sure what's up with CI. |
|
Amen, merged. |
Fixes #328
TBH I'm not sure if I missed something. The two methods are not like-for-like but
conversations_listseems a reasonable drop-in:channels_list, I would assume the default would filter out all the non-channel objects (e.g. IMs)For comparison: