More efficient name-to-id translation

[eizengan]

The conversations_id method paginates with default options over conversations.list responses to find a conversation with a particular name. The conversations.list endpoint has a Tier 2 rate limit, which - when combined with the low default page size - can cause just a single call in a workspace with 2000+ channels to result in rate limiting. This change enables use of non-default page sizes within conversations_id (and id_for, more generally), thus reducing the risk of rate limiting within such workspaces.

[dblock]

Thanks, this is a good change. However you should implement it in a way that does not change the existing functionality.

I think we don't want to have to pass the limit all over the place, but to configure it globally and per/client, so follow the existing settings pattern to add it. The default should be no changes, making the API calls as today.

[eizengan]

I think we don't want to have to pass the limit all over the place

It's only passed in the one spot, and should probably remain there; if we defer until we're inside id_for we either:

• lose the context of this being a conversations_id call instead of e.g. users_id, which has a different maximum page size
• have to build an understanding of the caller inside id_for, (i.e. branch on enum_method)

...implement it in a way that does not change the existing functionality...
...configure it globally and per/client...

Amended, pushed. How do we feel about the name conversations_id_page_size?

The config change does create an unusual codependency between Slack::Web::Api::Mixins::Conversations and Slack::Web::Config, as illustrated by this failing test run. The second commit here provides one possible solution.

[dblock]

We are still changing behavior by passing in a page size (100) by default. The code should not be passing anything into limit as before by default to avoid breaking backwards compatibility. I understand this may be the Slack default, or we have default_page_size, but we are relying on this knowledge here.

Stepping back, I propose we add id_for_page_size instead of conversations_id_page_size and implement this inside id_for. It will be simpler and cleaner. Unless you have a reason why this may be a bad idea?

And I'd like to be able to write this, overriding whatever the global id_for_page_size was set to (needs tests):

conversations_id(channel: 'C1234', limit: 10)

This will require passing the limit through, we may want to pass all of options down or add a single limit/page_size parameter.

Sorry to be a pest, hang in here with me :)

[eizengan]

We are still changing behavior by passing in a page size (100) by default.

I think you refer to theh explicit 100 I used to appease the test suite? If so, I'd overlooked the fact we were already doing some stubbing there. Oops! Switched to a stub - no more 100.

Stepping back, I propose we add id_for_page_size instead of conversations_id_page_size and implement this inside id_for. It will be simpler and cleaner. Unless you have a reason why this may be a bad idea?

My only major concern is hypothetical: there might be scenarios where no single page size is good for all translations using id_for. I think we only use this to translate users and channels at present, so my concern may be unwarranted. We can easily pivot to a single id_for_page_size config value if desired.

And I'd like to be able to write this, overriding whatever the global id_for_page_size was set to (needs tests):

conversations_id(channel: 'C1234', limit: 10)

I like it! Coming shortly...

[coveralls]

Pull Request Test Coverage Report for Build 15024095954

Details

• 35 of 35 (100.0%) changed or added relevant lines in 6 files are covered.
• 4 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.02%) to 90.422%

---

Files with Coverage Reduction	New Missed Lines	%
lib/slack/real_time/stores/store.rb	4	77.98%

Totals
Change from base Build 14347505748:	-0.02%
Covered Lines:	5853
Relevant Lines:	6473

---

💛 - Coveralls

[eizengan]

I've added a couple commits to implement the requested limit parameter and to extend configuration to users_id. We can instead consolidate those config options into a single id_for_page_size quite easily if that's your preference.

[dblock]

Much better!

Add tests for the default value and I think we need to rename things.

[dblock]

I think either we just add one more limit parameter to the already long list of arguments or change the signature to just id_for(options)? WDYT?

If you think both are a bad idea, at a minimum let's rename enum_method_options to just options to be future-proof?

[eizengan]

Add tests for the default value

Do the changes in the first new commit suffice? I'm not 100% sure on where these should go otherwise. Tests for page size are not present near the other config value tests.

I think either we just add one more limit parameter to the already long list of arguments or change the signature to just id_for(options)? WDYT?

While considering this I noticed a Problem; these methods are often used inside endpoint methods which - as a result of our change - unintentionally pass limit arguments intended for the parent call. I've tentatively renamed our new parameter id_limit to avoid this.

A single options parameter feels fragile to me since it would need exactly the right keys to work correctly. IMO keyword arguments feel like a good choice here.

The second new commit contains changes related to the above issues.

[dblock]

Are we changing default behavior for default page size still, or does it get called with default_page_size anyway today?

[dblock]

This is a change in default behavior isn't it? This test doesn't pass without the changes.

[eizengan]

Ah, yes - the old code relies on the default from within the paginator. Hash#compact should suffice to fix this if the defaults are nil; see what you think

[eizengan]

Something funky going on in tests - lots of

NameError:
  undefined method 'parse' for class '#<Class:CGI>'

followed by a segfault. Don't have time to dig in at the moment; if you do and hit paydirt, please let me know!

[eizengan]

Modified your CI so e.g. when ruby-head segfaults during tests Coveralls doesn't panic over the missing test info; ready for re-review

[dblock]

Looks great! Thanks for hanging in here with me.

[eizengan]

Thanks for hanging in here with me

Not at all; thanks for ensuring additions to this gem are appropriately refined!