-
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
India and Richelle - Water #12
base: master
Are you sure you want to change the base?
Conversation
…options in driver code
…o run added options
…space & updated select channel & user accordingly
… send message to users
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. |
✔️, nice work here |
Practices clean code. lib/slack.rb only interacts with Workspace to show a separation of responsibilities. Complex logic is broken into smaller helper methods. |
✔️ |
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. | ✔️ |
Practices writing tests. The User , Channel , and Workspace classes have unit tests. |
✔️, however User and Channel are pretty skimpy. |
Practices writing tests. There are tests for sending messages (the location of these tests may differ, but is likely in Recipient ) |
✔️, no tests for invalid or unselected channels/users. |
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
Overall Feedback | Criteria | yes/no |
---|---|---|
Green (Meets/Exceeds Standards) | 7+ in Code Review && 3+ in Functional Requirements | ✔️ |
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 | ✅ |
Summary
Nice work, you hit the learning goals here. Well done. Take a look at my comments and let me know what questions you have.
@@ -4,3 +4,4 @@ | |||
|
|||
# Ignore environemnt variables | |||
.env | |||
.idea/ |
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.
Thank you! However also please add coverage, it makes for a smaller PR.
.idea/ | |
.idea/ | |
coverage |
def self.list_all | ||
base_url = "https://slack.com/api/conversations.list" | ||
|
||
response = self.get(base_url) |
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 helper method
@@ -0,0 +1,45 @@ | |||
class SlackApiError < StandardError; end | |||
|
|||
class Recipient |
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.
Great abstract class! 😍
def select_user(input) | ||
@selected = @users.find { |user| user.name == input } | ||
if @selected | ||
return @selected | ||
else | ||
@selected = @users.find { |user| user.slack_id == input } | ||
end | ||
return @selected | ||
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.
Notice that select user and channel do the same thing, just to different arrays, can you make a helper method?
@@ -0,0 +1,17 @@ | |||
require_relative 'test_helper' | |||
|
|||
describe "Channel class" 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 have tests for the attr_reader attributes for channels, and the constructor.
@@ -0,0 +1,26 @@ | |||
require_relative 'test_helper' | |||
|
|||
describe "User class" 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 have tests for the constructor/attr_reader methods.
expect(@workspace.show_details).must_equal "This user's id is: USLACKBOT, the username is: slackbot and their real name is: Slackbot. They have a status emoji of: N/A and their status text is: N/A" | ||
end | ||
|
||
it "can post message to channel and return true" 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.
What about posting messages when there is no selected channel/user?
Assignment Submission: Slack CLI
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection