-
Notifications
You must be signed in to change notification settings - Fork 31
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
Water - Kayla and Mackenzie #15
base: master
Are you sure you want to change the base?
Conversation
…l to user-test file
git push
…ecause if string is only returned and not 'putsed' it won't show up in slack.rb.
…could run, added comments on tests as necessary
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.
Slack CLI
Major Learning Goals/Code Review
Criteria | yes/no, and optionally any details/lines of code to reference |
---|---|
Practices best practices working with APIs. The .env is not checked into git, and no API token was directly used in the Ruby code without ENV . |
✔️ |
Practices error handling with APIs. For all pieces of code that make an API call, it handles API requests that come back with errors/error status codes appropriately. | ✔️ |
Implements inheritance and inheritance idioms. There is a Recipient class. User and Channel inherit from Recipient . In Recipient , there are appropriate methods defined that are used in both User and Channel . Some may be implemented. Some may be template methods. |
✔️ |
Practices clean code. lib/slack.rb only interacts with Workspace to show a separation of responsibilities. Complex logic is broken into smaller helper methods. |
✔️ See in-line comment for suggestions how to break out methods further. |
Practices instance methods vs. class methods appropriately. The methods to list all Channels or Users is likely a class method within those respective classes. | ✔️ |
Practices best practices for testing. The project has and uses VCR mocking when running tests, and can run offline. | Some of the tests have errors or fail because the cassettes were checked in, and thus my test data is different. |
Practices writing tests. The User , Channel , and Workspace classes have unit tests. |
✔️ |
Practices writing tests. There are tests for sending messages (the location of these tests may differ, but is likely in Recipient ) |
✔️ |
Practices git with at least 15 small commits and meaningful commit messages | ✔️ |
Functional Requirements
Functional Requirement | yes/no |
---|---|
As a user of the CLI program, I can list users and channels | ✔️ |
As a user of the CLI program, I can select users and channels | ✔️ |
As a user of the CLI program, I can show the details of a selected user or channel | ✔️ |
As a user of the CLI program, when I input something inappropriately, the program runs without crashing | ✔️ |
Overall Feedback
Excellent job overall. This code is well-written and well-tested. It is clear that the learning goals around understanding the request/response cycle, consuming an API, and implementing a design using inheritance from scratch were all met. I've left a few inline comments for you to review below. Keep up the hard work!
Overall Feedback | Criteria | yes/no |
---|---|---|
Green (Meets/Exceeds Standards) | 7+ in Code Review && 3+ in Functional Requirements | ✔️ |
Yellow (Approaches Standards) | 6+ in Code Review && 2+ in Functional Requirements | |
Red (Not at Standard) | 0-5 in Code Review or 0,1 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging |
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
Quality | Yes? |
---|---|
Perfect Indentation | ✅ |
Elegant/Clever | ✅ |
Descriptive/Readable | ✅ |
Concise | ✅ |
Logical/Organized | ✅ |
until user_input == "7" || user_input == "quit" | ||
puts options | ||
user_input = gets.chomp.downcase | ||
|
||
case user_input | ||
when "1", "list users" | ||
workspace.list_users | ||
when "2", "list channels" | ||
workspace.list_channels | ||
when "3", "select user" | ||
puts "Please enter User Name or ID" | ||
user_identifier = gets.chomp.downcase | ||
workspace.select_user(user_identifier) | ||
when "4", "select channel" | ||
puts "Please enter Chanel Name or ID" | ||
channel_identifier = gets.chomp.downcase | ||
workspace.select_channel(channel_identifier) | ||
when "5", "details" | ||
if workspace.selected_recipient == nil | ||
puts "Please select a channel or user first to view details" | ||
else | ||
puts workspace.find_details | ||
end | ||
when "6", "send message" | ||
if workspace.selected_recipient == nil | ||
puts "Please select a channel or user first to send message" | ||
else | ||
puts "Send a message!" | ||
message = gets.chomp | ||
workspace.send_msg(message) | ||
puts "\nMessage sent!" | ||
end | ||
end | ||
end | ||
puts "Thank you for using the Ada Slack CLI" | ||
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.
Consider wrapping this functionality into its own method.
puts "Welcome to the Ada Slack CLI!" | ||
puts "Welcome to the Ada Slack CLI!\n\n" | ||
workspace = Workspace.new | ||
|
||
# TODO project | ||
puts "There are #{workspace.channels.count} channels in this workspace" | ||
puts "There are #{workspace.users.count} users in this workspace." | ||
puts | ||
puts "*" * 40 |
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.
Consider wrapping this set-up functionality into its own method.
return response | ||
end | ||
|
||
def self.send_msg(message, id) |
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.
Consider making send_msg
an instance method that's called on an instance of a User
or Channel
. In this way, id
does not need to be an argument.
} | ||
) | ||
|
||
unless response.code == 200 && response.parsed_response["ok"] |
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 work error handling.
headers: { 'Content-Type' => 'application/x-www-form-urlencoded'} | ||
) | ||
|
||
unless response.code == 200 && response.parsed_response["ok"] |
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 work error handling with a custom exception.
Assignment Submission: Slack CLI
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection