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

Log when methods are deprecated #329

Merged
merged 8 commits into from
Jun 22, 2020

Conversation

wasabigeek
Copy link
Contributor

Follow-up to #320 - log when a method is / will be deprecated.

There are some new methods that were added as well from the latest slack-api-ref, I'll open a new PR.

@dblock
Copy link
Collaborator

dblock commented Jun 19, 2020

Add a spec similar to undocumented methods.

I think we have to decide whether the method is or will be deprecated. I would say "is". If the method stops working on a certain date we should extract that into slack-api-ref and add the message in the warning along the lines of "it will stop working on ...".

@wasabigeek
Copy link
Contributor Author

What do you think about linking to the specific slack API instead, or copying the entire message? The deprecation warning actually has two dates e.g. https://api.slack.com/methods/channels.list

This method is deprecated. It will stop functioning in February 2021 and will not work with newly created apps after June 10th, 2020. Learn more.
Please use these methods instead:
conversations.list
users.conversations

@dblock
Copy link
Collaborator

dblock commented Jun 19, 2020

What do you think about linking to the specific slack API instead, or copying the entire message? The deprecation warning actually has two dates e.g. https://api.slack.com/methods/channels.list

This method is deprecated. It will stop functioning in February 2021 and will not work with newly created apps after June 10th, 2020. Learn more.
Please use these methods instead:
conversations.list
users.conversations

Even better. I would use the message as is.

@wasabigeek
Copy link
Contributor Author

There are some new methods picked up with the updated submodule, I haven't committed them - barring failed tests, will open a separate PR for them.

let(:client) { described_class.new }

it 'produces a warning' do
expect(client.logger).to receive(:warn).with(/^channels.archive: This method is deprecated./)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check for a list of alternate methods here.

@dblock
Copy link
Collaborator

dblock commented Jun 22, 2020

Needs a rubocop -a ; rubocop --auto-gen-config. Looks good!

Comment on lines +98 to 106
# Offense count: 77
# Configuration parameters: .
# SupportedStyles: have_received, receive
RSpec/MessageSpies:
EnforcedStyle: receive

# Offense count: 94
# Offense count: 95
RSpec/MultipleExpectations:
Max: 5
Copy link
Contributor Author

@wasabigeek wasabigeek Jun 22, 2020

Choose a reason for hiding this comment

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

EDITED

Ok I managed to get by this offence by doing this:

        logger = instance_double(Logger)
        allow(client).to receive(:logger).and_return(logger)
        allow(client).to receive(:post)

        expect(logger).to receive(:warn).with(/
          ^channels\.archive:\ This\ method\ is\ deprecated
          .+
          Alternative\ methods:\ conversations\.archive\.
        /x)
        client.channels_archive(channel: 'test')

Do you think I should do so? If not, could we ignore these style cops completely :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think ignoring expectations is fine. I usually just -a everything and selectively fix if I feel like it.

@dblock dblock merged commit 318796f into slack-ruby:master Jun 22, 2020
@dblock
Copy link
Collaborator

dblock commented Jun 22, 2020

Merged, thank you.

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.

2 participants