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

Conversation

alto
Copy link
Member

@alto alto commented Sep 1, 2021

No description provided.

@alto alto added the bug label Sep 1, 2021
@alto alto requested a review from kylorhall September 1, 2021 07:46
@alto alto self-assigned this Sep 1, 2021
Copy link

@kylorhall kylorhall left a comment

Choose a reason for hiding this comment

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

LGTM, just comments about comments pretty much.

@@ -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

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?

get('/bins?max-results=500').map do |bin_data|
per_page = 500
data = (0..1).inject([]) do |list, page|
list += get("/bins?max-results=#{per_page}&page-token=#{page * 500}")

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.

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

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|

@@ -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)

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.

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

@alto alto merged commit fdd4a26 into master Sep 1, 2021
@alto alto deleted the add_support_for_more_than_500_bins branch September 1, 2021 21:46
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants