-
Notifications
You must be signed in to change notification settings - Fork 27
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
Ports - Kasey and Elle #15
base: master
Are you sure you want to change the base?
Conversation
…s together (require statements) and circular reference with minitest.
…s selected' to not throw an error and crash. Worked on making 'send message' work on my machine; not done.
…kind of recipient (User or Channel) and set query parameters accordingly. Also put in a couple of catches for user or channel being nil, which can happen if you do 'select user/channel' and enter an invalid selection, and then select 'send message'. This now gets caught and redirects the user to the main prompt instead of crashing. TO DO: One test is now throwing an error. Need to fix test, the code works.
…due to refactoring or being duplicates.
slack.rbWhat We're Looking For
|
|
||
class SlackError < StandardError; end | ||
|
||
class Slack |
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.
This class would be better named SlackWorkspace
since slack can include multiple workspaces.
Also it would be good to wrap everything in a module
get_lists | ||
end | ||
|
||
def get_lists() |
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.
Just notice here how the loops look almost identical. This is a good indication that you can dry things up using duck typing / polymorphism.
end | ||
|
||
url = "https://slack.com/api/chat.postMessage" | ||
if recipient.class == Channel |
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.
The if
and elsif
here are identical, so you just need to check to see if recipient
is not nil
.
|
||
def main | ||
puts "Welcome to the Ada Slack CLI!" | ||
slack = Slack.new |
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.
It would be better to have main
and the Slack
class in separate files.
|
||
describe "select users and channels" do | ||
it "select a user" do | ||
VCR.use_cassette("slack_query") do |
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.
You should also test with an invalid user.
end | ||
end | ||
|
||
it "select a channel" do |
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.
ditto here
end | ||
end | ||
|
||
describe "select users and channels" do |
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.
This describe seems to include seeing details rather than select user and channels.
You should also test what happens when no user or channel is selected.
You should test with both a user and channel.
end | ||
end | ||
|
||
describe "send_msg" do |
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.
Good edge-case tests here.
@@ -0,0 +1,15 @@ | |||
require_relative "test_helper" |
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.
This file doesn't seem to fit here. Also where are tests on the User
class.
} | ||
url = "https://slack.com/api/conversations.list?" | ||
response = HTTParty.get(url, query: query_parameters) | ||
response["channels"].each do |x| |
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.
.map could also be used here to give you an array of Channel instances
slack.rb
Congratulations! You're submitting your assignment!
You and your partner should collaborate on the answers to these questions.
Comprehension Questions