Introduce SlackExplorer
#33
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This class was design only for working with SimpleWriter, so nesting it makes this clear.
This class used to know too much about the app-specific needs. It's now a thin wrapper around the Slack API client gem. A major change though, is that it automatically fetches the message author real name, so it issues an additional API call for each message. To remedy the performance hits, all the author names are requested in parallel, with a cache to avoid unnecessary requests.
This class is responsible for "exploring" Slack and "mining" interesting messages. Those messages, later become a blog post. Now that we have a simple Slack client, we can put the business logic in the explorer. This also opens the door for more explorers in the future, like a Twitter explorer.
MatheusRich
commented
Sep 7, 2023
elias19r
reviewed
Sep 8, 2023
There was a problem hiding this comment.
GOLD_EMOJI = "rupee-gold"can be deleted from Slack::Client because it has been moved to SlackExplorer
Comment on lines
+150
to
+158
| t0 = Process.clock_gettime(Process::CLOCK_MONOTONIC) | ||
| slack.search_messages(search_query) | ||
| total_elapsed_time = Process.clock_gettime(Process::CLOCK_MONOTONIC) - t0 | ||
|
|
||
| # We have to sum the search and user info query times because they run | ||
| # sequentially. Both user info queries run in parallel, so we don't have | ||
| # to sum them twice. | ||
| overhead = 0.1 | ||
| expect(total_elapsed_time).to be_within(overhead).of(search_query_time + user_info_query_time) |
There was a problem hiding this comment.
Nice! I had no idea how to test this
There was a problem hiding this comment.
minor: It might be worth it to assert the following:
expect(sleepy_slack_client).to have_received(:search_messages).with({query: search_query}).once
expect(sleepy_slack_client).to have_received(:users_info).with({user: user1.id}).once
expect(sleepy_slack_client).to have_received(:users_info).with({user: user2.id}).onceI usually add those assertions to make sure the stub/mocks have actually been called.
In other words, without those assertions, this implementation of search_messages passes the test :D
module GoldMiner
class Slack::Client
# ...
def search_messages(query)
sleep 1.5
end
Comment on lines
+30
to
+34
| def deep_open_struct(hash) | ||
| require "ostruct" | ||
|
|
||
| JSON.parse hash.to_json, object_class: OpenStruct | ||
| end |
There was a problem hiding this comment.
TIL about JSON.parse with object_class: OpenStruct, nice!
elias19r
approved these changes
Sep 8, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While coding #25, I noticed (as correctly pointed out by @elias19r) some weird dependencies in the app: The Slack Client knew too much about the business logic. This PR is a first move in a new direction. More changes will come in the future.
Here are the major changes:
Rewrite
Slack::Client:It's now a thin wrapper around the Slack API client gem.
author real name, so it issues an additional API call for each message.
To remedy the performance hits, all the author names are requested in
parallel, with a cache to avoid unnecessary requests.
Introduce
GoldMiner::SlackExplorermessages. Those messages, later become a blog post.
in the explorer. This also opens the door for more explorers in the future,
like a Twitter explorer.