-
-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce SlackExplorer
#33
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GOLD_EMOJI = "rupee-gold"
can be deleted from Slack::Client
because it has been moved to SlackExplorer
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I had no idea how to test this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}).once
I 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
def deep_open_struct(hash) | ||
require "ostruct" | ||
|
||
JSON.parse hash.to_json, object_class: OpenStruct | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about JSON.parse
with object_class: OpenStruct
, nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I like the SlackExplorer
that knows how to explore Slack, using code from Slack
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::SlackExplorer
messages. 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.