Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Add support for more than 500 bins #20

Merged
merged 1 commit into from
Sep 1, 2021
Merged

Add support for more than 500 bins #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/vimaly/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,11 @@ def bin(name_or_regex)

def bins
@bins ||= begin
get('/bins?max-results=500').map do |bin_data|
per_page = 500
data = (0..1).inject([]) do |list, page|
Copy link

@kylorhall kylorhall Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So this breaks when I add 450 more bins? Sounds good. Can you make it break harder so if we have 1000 bins we're forced to move away…as a failsafe?

list += get("/bins?max-results=#{per_page}&page-token=#{page * 500}")
Copy link

Choose a reason for hiding this comment

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

LGTM, their API is a bit odd…page-token what does that mean?

Assuming it works.

end
data.map do |bin_data|
Comment on lines +92 to +93
Copy link

Choose a reason for hiding this comment

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

I guess I might suggest this and discarding the data constant altogether. In Javascript I would, but alas. Ignore me!

Suggested change
end
data.map do |bin_data|
.map do |bin_data|

Bin.new(bin_data['_id'], bin_data['name'])
end
end
Expand Down
6 changes: 5 additions & 1 deletion test/client_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,15 @@ class ClientTest < Minitest::Test
client = Vimaly::Client.new('company_id', user_credentials: { username: 'username', password: 'password' })
bins = client.bins

assert_equal 2, bins.size
assert_equal 4, bins.size # API called twice now (2 pages)
Copy link

Choose a reason for hiding this comment

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

LGTM, so our Mocked API is returning [Alpha, Beta] for both requests? Eg. page=2 is still returning [Alpha, Beta]? Clearly a fake mock, but works for me.

Copy link

Choose a reason for hiding this comment

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

Something a bit more? Out-of-context I think it's confusing.

Suggested change
assert_equal 4, bins.size # API called twice now (2 pages)
# API called twice now (2x pages returning a mocked result of 2 bins)
assert_equal 4, bins.size

assert_equal 'Alpha', bins[0].name
assert_equal 1, bins[0].id
assert_equal 'Beta', bins[1].name
assert_equal 2, bins[1].id
assert_equal 'Alpha', bins[2].name
assert_equal 1, bins[2].id
assert_equal 'Beta', bins[3].name
assert_equal 2, bins[3].id
end
end

Expand Down