Branches - Eve and Mira by calopter · Pull Request #3 · Ada-C12/slack-cli

calopter · 2019-09-13T19:31:36Z

slack.rb

Congratulations! You're submitting your assignment!

You and your partner should collaborate on the answers to these questions.

Comprehension Questions

Question	Answer
How did you go about exploring the Slack API? Did you learn anything that would be useful for your next project involving an API?	We read the documentation for the endpoints we were interested in and used postman to verify a basic request.
Give a short summary of the request/response cycle. Where does your program fit into that scheme?	Our program makes requests and uses corresponding responses to build data.
How does your program check for and handle errors when using the Slack API?	We raise a SlackAPIError anytime we get a non-successful response code.
Did you need to make any changes to the design work we did in class? If so, what were they?	We made no changes.
Did you use any of the inheritance idioms we've talked about in class? How?	Recipient defines template methods to be implemented by child classes.
How does VCR aid in testing a program that uses an API?	It allows to avoid repeating requests every time we run the tests.

…lly tested in Recipient class

kaidamasaki · 2019-09-20T23:48:05Z

slack.rb

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene (no slack tokens)	Yes.
Comprehension questions	Yes.
Functionality
List users/channels	Yes.
Select user/channel	Yes.
Show details	Yes.
Send message	Yes.
Program runs without crashing	Yes.
Implementation
API errors are handled appropriately	Mostly.
Inheritance model matches in-class activity	Yes.
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately	Yes.
Methods are used to break work down into simpler tasks	Yes.
Class and instance methods are used appropriately	Yes.
Tests written for User functionality	Yes.
Tests written for Channel Functionality	Yes.
Tests written for sending a message	Yes.
Overall	Great job! You have some issues with omitting error messages in exceptions but overall things look really good. Also, I liked how succinct your answers to the comprehension questions were.

kaidamasaki · 2019-09-20T23:48:07Z

slack.rb

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene (no slack tokens)	Yes.
Comprehension questions	Yes.
Functionality
List users/channels	Yes.
Select user/channel	Yes.
Show details	Yes.
Send message	Yes.
Program runs without crashing	Yes.
Implementation
API errors are handled appropriately	Mostly.
Inheritance model matches in-class activity	Yes.
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately	Yes.
Methods are used to break work down into simpler tasks	Yes.
Class and instance methods are used appropriately	Yes.
Tests written for User functionality	Yes.
Tests written for Channel Functionality	Yes.
Tests written for sending a message	Yes.
Overall	Great job! You have some issues with omitting error messages in exceptions but overall things look really good. Also, I liked how succinct your answers to the comprehension questions were.

kaidamasaki · 2019-09-20T23:32:45Z

lib/recipient.rb

+  class SlackApiError < Exception; end
+
+  def initialize(slack_id:, name:)
+    raise ArgumentError unless (slack_id && name)


This is redundant, keyword arguments without defaults are required.

kaidamasaki · 2019-09-20T23:33:15Z

lib/recipient.rb

+  end
+
+  def self.list
+    raise NotImplementedError, "template method"


Template method! 🎉

kaidamasaki · 2019-09-20T23:37:23Z

lib/recipient.rb

+
+  def self.get(url, params)
+    response = HTTParty.get(url, query: params)
+    raise SlackApiError unless response['ok']


It would be helpful to include the error in the message and also check the response code.

Suggested change

raise SlackApiError unless response['ok']

raise SlackApiError.new(response["error"]) unless response['ok'] && response.code == 200

kaidamasaki · 2019-09-20T23:38:26Z

lib/recipient.rb

+      raise SlackApiError
+    end
+
+    return true


Returning true is redundant here, since you know this succeeded if it returns.

kaidamasaki · 2019-09-20T23:40:12Z

lib/slack.rb

+        begin
+          workspace.send_message 
+        rescue Recipient::SlackApiError
+          puts "Unable to send message\n\n"


If you'd include a message you could print a more descriptive error here.

rescue Recipient::SlackApiError => api_error puts "Unable to send message #{api_error.message}\n\n"

kaidamasaki · 2019-09-20T23:42:39Z

lib/workspace.rb

+
+  def send_message
+    if selected
+      puts "Please enter message to send to #{selected.name}: "


Printing belongs in your driver code (slack.rb). Including it here makes this difficult to unit-test.

calopter and others added 22 commits September 10, 2019 13:36

verifies authorization

2317503

Create Recipient class and add test file

c8ead4c

Create User class and add test file

7c7af5e

Create Channel class and add test file

084aab1

establish Workspace class and tests

554c02c

implemented .get

9481b4a

implemented User.list

b1b9a01

Update Recipient class: .get raises error for bad responses

3cc9287

Implement .list for Channel class and update tests

1d7e403

Implement details method for User class and update test file

c696212

Implement details method for Channel class and update test file

62ee8b7

implements show_details

f84c63a

implements wave 1 cli

542eaf6

refactors show_details to generalize

a93dbfa

Implement find_recipient method for Workspace class

c98a559

implements select_user and select_channel

d37591c

Update slack.rb to have more options

3abe4b6

implements Recipient#send_message

e0424d3

Implement send_message for Workspace class

d831af9

implements send message behavior in main

3b17b52

Remove send_message tests for Workspace since send_message is interna…

f241e43

…lly tested in Recipient class

Clean up code and improve UI

9acc681

kaidamasaki reviewed Sep 20, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branches - Eve and Mira#3

Branches - Eve and Mira#3
calopter wants to merge 22 commits intoAda-C12:masterfrom
calopter:master

calopter commented Sep 13, 2019

Uh oh!

kaidamasaki commented Sep 20, 2019

Uh oh!

kaidamasaki commented Sep 20, 2019

Uh oh!

kaidamasaki Sep 20, 2019

Uh oh!

kaidamasaki Sep 20, 2019

Uh oh!

kaidamasaki Sep 20, 2019

Uh oh!

kaidamasaki Sep 20, 2019

Uh oh!

kaidamasaki Sep 20, 2019

Uh oh!

kaidamasaki Sep 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	raise SlackApiError unless response['ok']
	raise SlackApiError.new(response["error"]) unless response['ok'] && response.code == 200

Conversation

calopter commented Sep 13, 2019

slack.rb

Comprehension Questions

Uh oh!

kaidamasaki commented Sep 20, 2019

slack.rb

What We're Looking For

Uh oh!

kaidamasaki commented Sep 20, 2019

slack.rb

What We're Looking For

Uh oh!

kaidamasaki Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

kaidamasaki Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

kaidamasaki Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

kaidamasaki Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

kaidamasaki Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

kaidamasaki Sep 20, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants