Skip to content
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

Alice/Ting-Yi - SlackCli #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

aliceboone
Copy link

Assignment Submission: Slack CLI

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

Reflection

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? It was really great and fun! We learn how to create request to get and post data pro the API, use token as a key, we believe everything we learn will be useful in your next project involving an API.

Give a short summary of the request/response cycle. Where does your program fit into that scheme? | When we need some information from the Server, our CLI code is a client and we use API with a Key and URL to retrieve the information from server. Our program fit in the scheme when we make a request go get user and channels data from the API and we get the response we ask for.

How does your program check for and handle errors when using the Slack API? | Our program check if the response code is correct and the parsed response is ok, and check if our token is valid before loading data, if not we raise a error.

How did the design and organization of your project change over time? | At the beginning was a little confused, but after talking and going through the questions it get clarified and we realize we had an inheritance between the recipient class, channels and users, and how we could organize the methods properly.

Did you use any of the inheritance idioms we've talked about in class? How? | Yes, we create a class Recipient as our abstract class, and we created the methods in recipient class as our template methods and applied them in our classes User and Channels.

How does VCR aid in testing a program that uses an API? | VCR aid is really helpful and saves us from being banned by our API token.

@jmaddox19
Copy link

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. The POST request has error handling but not the GET request. (See inline comments)
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. ✔️ For the most part, although Recipient is missing a template method for details. See inline comments.
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. ✔️
Practices writing tests. There are tests for sending messages (the location of these tests may differ, but is likely in Recipient) ✔️ Yes, but only for the simple success case. Edge case testing is needed.
Practices git with at least 15 small commits and meaningful commit messages I would highly recommend committing much more frequently than once or twice per wave, especially as the projects continue to get bigger and bigger.

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 ✔️
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

Additional Feedback

Great work!! Y'all made a whole program that interacts with Slack in real time from the command terminal!!
I hope y'all have gotten to celebrate and appreciate how fun of an accomplishment that is!!
The program all works together as expected and the code is very readable!

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

Comment on lines +20 to +21
response = HTTParty.get(url, query: query_items)
return response

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great error handling on line 42! This could use something similar here

end

# Define how channel details will be displayed
def details

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this method should be defined for all children of Recipient, it is appropriate for there to be a template method called details in the Recipient class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants