Don't send an empty limit query parameter for Channel#get_reactions #39

kaine119 · 2021-03-06T17:24:50Z

Summary

Fixes #38.
Discord tries to parse the limit parameter as an integer, if nothing is sent a HTTP 400 is returned. This sets a default to the maximum instead of the usual 25, to reduce the number of requests needed to get all users when limit: nil is passed.

Added

Specs for Message#reacted_with and API::Channel.get_reactions

Changed

API::Channel.get_reactions: the query param limit defaults to 100 instead of the empty string when limit is nil.

Daniel-Worrall · 2021-03-06T17:27:12Z

Been talking about this one in the discord before submission.
LGTM, would love some specs, and let's get CI before any merge

swarley · 2021-03-07T04:13:19Z

lib/discordrb/api/channel.rb

@@ -201,7 +201,7 @@ def delete_user_reaction(token, channel_id, message_id, emoji, user_id)
  # https://discord.com/developers/docs/resources/channel#get-reactions
  def get_reactions(token, channel_id, message_id, emoji, before_id, after_id, limit = 100)
    emoji = URI.encode_www_form_component(emoji) unless emoji.ascii_only?
-    query_string = "limit=#{limit}#{"&before=#{before_id}" if before_id}#{"&after=#{after_id}" if after_id}"
+    query_string = "limit=#{limit || 100}#{"&before=#{before_id}" if before_id}#{"&after=#{after_id}" if after_id}"


I would prefer to see this as

query_string = URI.encode_www_form({ limit: limit, before: before_id, after: after_id }.compact)

Please squash this thanks

Daniel-Worrall

lgtm so far. Just some nitpicky spec changes going forward. I'm aware some parts of the library also use these things I've criticised which is where you may have taken inspiration from, but it's on the agenda to go over the specs to make them more consistent, and document guidelines in a Contributing section

Daniel-Worrall · 2021-03-07T14:34:18Z

spec/api/channel_spec.rb

+    let(:message_id) { double('message_id') }
+    let(:before_id) { double('before_id') }
+    let(:after_id) { double('after_id') }
+
+    before do
+      allow(message_id).to receive(:to_s).and_return('message_id')
+      allow(before_id).to receive(:to_s).and_return('before_id')
+      allow(after_id).to receive(:to_s).and_return('after_id')
+


and here

Suggested change

let(:message_id) { double('message_id') }

let(:before_id) { double('before_id') }

let(:after_id) { double('after_id') }

before do

allow(message_id).to receive(:to_s).and_return('message_id')

allow(before_id).to receive(:to_s).and_return('before_id')

allow(after_id).to receive(:to_s).and_return('after_id')

let(:message_id) { double('message_id', to_s: 'message_id') }

let(:before_id) { double('before_id', to_s: 'before_id') }

let(:after_id) { double('after_id', to_s: 'after_id') }

before

Daniel-Worrall · 2021-03-07T14:34:25Z

spec/api/channel_spec.rb

+  let(:token) { double('token') }
+  let(:channel_id) { instance_double(String, 'channel_id') }
+
+  before do
+    allow(token).to receive(:to_s).and_return('bot_token')
+    allow(channel_id).to receive(:to_s).and_return('channel_id')
+  end


We can move these method stubs inside the double initialiser

Suggested change

let(:token) { double('token') }

let(:channel_id) { instance_double(String, 'channel_id') }

before do

allow(token).to receive(:to_s).and_return('bot_token')

allow(channel_id).to receive(:to_s).and_return('channel_id')

end

let(:token) { double('token', to_s: 'bot_token') }

let(:channel_id) { instance_double(String, 'channel_id', to_s: 'channel_id') }

Daniel-Worrall · 2021-03-07T14:39:16Z

spec/api/channel_spec.rb

+
+    it 'sends requests' do
+      Discordrb::API::Channel.get_reactions(token, channel_id, message_id, 'test', before_id, after_id, 27)
+      expect(Discordrb::API).to have_received(:request)


while have_received is valid spec'ing known as using spies, I'd prefer we stay consistent and put the expectations before the call

Daniel-Worrall · 2021-03-07T14:39:28Z

spec/api/channel_spec.rb

+
+    it 'percent-encodes emoji' do
+      Discordrb::API::Channel.get_reactions(token, channel_id, message_id, "\u{1F44D}", before_id, after_id, 27)
+      expect(Discordrb::API).to have_received(:request)


Daniel-Worrall · 2021-03-07T14:39:37Z

spec/api/channel_spec.rb

+
+    it 'uses the maximum limit of 100 if nil is provided' do
+      Discordrb::API::Channel.get_reactions(token, channel_id, message_id, 'test', before_id, after_id, nil)
+      expect(Discordrb::API).to have_received(:request)


Daniel-Worrall · 2021-03-07T14:40:32Z

spec/data/message_spec.rb

+    it 'calls the API method' do
+      message.reacted_with('\u{1F44D}', limit: 27)
+
+      expect(Discordrb::API::Channel).to have_received(:get_reactions)


Daniel-Worrall · 2021-03-07T14:40:49Z

spec/data/message_spec.rb

+    it 'defaults to a limit of 100' do
+      message.reacted_with('\u{1F44D}')
+
+      expect(Discordrb::API::Channel).to have_received(:get_reactions)


Daniel-Worrall · 2021-03-07T14:41:03Z

spec/data/message_spec.rb

+
+      message.reacted_with(emoji)
+
+      expect(Discordrb::API::Channel).to have_received(:get_reactions)


Daniel-Worrall

lgtm.

Was gonna say "let's see about CI" but it seems to be working now :)

swarley

Just one small fix and then things should be good to go!

lib/discordrb/api/channel.rb

swarley · 2021-04-29T00:03:23Z

Thank you!

Don't send an empty limit query parameter for Channel#get_channel

0832ed2

Add spec for Message#reacted_with

852958c

kaine119 force-pushed the get_reactions_limit_default branch from eb458d7 to 852958c Compare March 6, 2021 18:54

Update Message#reacted_with spec

d400366

swarley reviewed Mar 7, 2021

View reviewed changes

kaine119 added 7 commits March 7, 2021 12:22

Properly stub API::Channel to return test user data based on

31a3098

Appease the cop

d53d066

Use a query string builder instead of interpolating it

0e82cd1

Default limit query param to 100

8f6a7a0

Add case for limit: nil

e1452d0

Add spec for API::Channel.get_reactions

356f190

Appease rubocop (again)

62e714c

Please squash this thanks

kaine119 requested a review from swarley March 7, 2021 07:46

Daniel-Worrall requested changes Mar 7, 2021

View reviewed changes

kaine119 added 2 commits March 8, 2021 14:46

Move method stubs into double initializer

97cbce7

Move mock call expectations in front of calls

dbb35ef

kaine119 requested a review from Daniel-Worrall March 8, 2021 06:50

Daniel-Worrall approved these changes Mar 8, 2021

View reviewed changes

swarley requested changes Apr 28, 2021

View reviewed changes

lib/discordrb/api/channel.rb Show resolved Hide resolved

swarley approved these changes Apr 29, 2021

View reviewed changes

swarley merged commit 504285e into shardlab:main Apr 29, 2021

Daniel-Worrall changed the title ~~Don't send an empty limit query parameter for Channel#get_channel~~ Don't send an empty limit query parameter for Channel#get_reactions Jun 1, 2021

Daniel-Worrall mentioned this pull request Jun 1, 2021

Channel#reacted_with limit:nil Bug - Again #60

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't send an empty limit query parameter for Channel#get_reactions #39

Don't send an empty limit query parameter for Channel#get_reactions #39

kaine119 commented Mar 6, 2021 •

edited

Loading

Daniel-Worrall commented Mar 6, 2021

swarley Mar 7, 2021

Daniel-Worrall left a comment

Daniel-Worrall Mar 7, 2021

Daniel-Worrall Mar 7, 2021

Daniel-Worrall Mar 7, 2021

Daniel-Worrall Mar 7, 2021

Daniel-Worrall Mar 7, 2021

Daniel-Worrall Mar 7, 2021

Daniel-Worrall Mar 7, 2021

Daniel-Worrall Mar 7, 2021

Daniel-Worrall left a comment

swarley left a comment

swarley commented Apr 29, 2021


		message.reacted_with(emoji)

		expect(Discordrb::API::Channel).to have_received(:get_reactions)

Don't send an empty limit query parameter for Channel#get_reactions #39

Don't send an empty limit query parameter for Channel#get_reactions #39

Conversation

kaine119 commented Mar 6, 2021 • edited Loading

Summary

Added

Changed

Daniel-Worrall commented Mar 6, 2021

Choose a reason for hiding this comment

Daniel-Worrall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel-Worrall left a comment

Choose a reason for hiding this comment

swarley left a comment

Choose a reason for hiding this comment

swarley commented Apr 29, 2021

kaine119 commented Mar 6, 2021 •

edited

Loading