Skip to content

Conversation

@eizengan
Copy link
Contributor

@eizengan eizengan commented Jun 24, 2025

Ran into another hitch with name-to-id translation: the gem is only capable of translating public channels.

This first pass does the minimum amount of work to make the feature happen. It has an obvious flaw, however, in that the continued addition of new arguments feels untenable (e.g. team_id or exclude_archived). Perhaps this is the last argument we need/want, and the first-pass solution is fine - or perhaps not. It's a good starting point for conversation, regardless. We can refine once I hear your thoughts.

@eizengan eizengan force-pushed the eizengan/nonpublic_channel_translation branch from ff70185 to ee06927 Compare June 24, 2025 14:42
@coveralls
Copy link

coveralls commented Jun 24, 2025

Pull Request Test Coverage Report for Build 15853774080

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 90.385%

Totals Coverage Status
Change from base Build 15252449956: 0.007%
Covered Lines: 5913
Relevant Lines: 6542

💛 - Coveralls

@dblock
Copy link
Collaborator

dblock commented Jun 25, 2025

Looks ok to me, what is the concern? Having to fetch another 2 parameters?

@eizengan
Copy link
Contributor Author

eizengan commented Jun 25, 2025

Looks ok to me, what is the concern? Having to fetch another 2 parameters?

A twofold concern:

  • we again solve the need for one specific parameter instead of providing a method of passing whatever parameters are needed
  • expansion to the other parameters in the same manner (i.e. more options.fetch(:id_*, nil)s) feels ugly

If we instead plucked options[:id_options] and forwarded that in its entirety that might be better, but it's also incompatible with my prior addition of id_limit without some massaging. If you're interested I can tentatively push a commit containing that change.

@dblock dblock merged commit cabf9d2 into slack-ruby:master Jun 26, 2025
12 checks passed
@dblock
Copy link
Collaborator

dblock commented Jun 26, 2025

The number of parameters passed here is limited, so I am ok with this. Want to open a PR for the rest of them?

@eizengan
Copy link
Contributor Author

Want to open a PR for the rest of them?

Sure - might as well while I'm in here; save someone else the trouble down the line

@eizengan
Copy link
Contributor Author

Closing the loop: the other additions are in #560

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.

3 participants