-
Notifications
You must be signed in to change notification settings - Fork 50
Space - Antonia #45
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
base: master
Are you sure you want to change the base?
Space - Antonia #45
Conversation
jmaddox19
left a comment
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 work! I'm impressed you got as much as you did with such little healthy time to work with!
I do see what you were saying about not having as much time to spend on tests as you would have liked.
I will continue to check-in on your testing as we get into it with JS. I recommend making that an area of focus while working on JS Adagrams so we can identify if there are still any gaps.
| if response.code != 200 || response['ok'] == false | ||
| raise SlackAPIError | ||
| 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.
This honestly is generally pretty sufficient! The one thing I would add is figuring out what part of the response gives a useful message explaining what went wrong and making sure to record that as a property of SlackAPIError.
If it's unclear what I mean by that, please ask!
| before do | ||
| @user = User.new(slack_id: "USLACKBOT", name: "slackbot", real_name: "Slackbot", status_text: "", status_emoji: "") | ||
| end | ||
| it "returns user details" 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 should be one indentation level further.
| end | ||
| 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.
There's actually a missing end here that I think results in unintentionally nested "describe" blocks. Fixing the indentation on line 18 would make that easier to catch.
I use a VSCode plugin called indent-rainbow that I really like to help me catch things like this. It works for ruby and JS!
| @user = User.new(slack_id: "USLACKBOT", name: "slackbot", real_name: "Slackbot", status_text: "", status_emoji: "") | ||
| end | ||
| it "returns user details" do | ||
| expect(@user.details).must_equal "Slack ID: #{@user.slack_id}, Name: #{@user.name}, Real Name: #{@user.real_name}, Status Text: #{@user.status_text}, Status Emoji: #{@user.status_emoji}" |
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 should be 2 indentation levels further.
|
|
||
| def self.list_all | ||
| #get the data | ||
| data = self.get("https://slack.com/api/users.list") |
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.
As a general rule, every API call should include some error handling. (There is always going to be the possibility of a network problem, the API url getting changed, etc.)
|
Sorry, I just realized that all the tests are written for you in the JS Adagrams project, so that won't be an opportunity to practice testing. |
Assignment Submission: Slack CLI
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection