Time - HannahT #47
Time - HannahT #47
Conversation
CheezItMan
left a comment
CheezItMan
left a comment
There was a problem hiding this comment.
Everything works and that's great it does what it's expected to do. Well done.
Suggestions for Improvement:
- Divide up the roles for each component a little more clearly.
- Like make Recipient-User-Channel do the API work and let Workspace manage them.
- Use some factory methods in
UserandChannellike we did in Grocery Store, rather than putting them in Workspace. It's not bad, as is, it just could be more clean that way. - A bit more through testing and make sure you're only testing what you need in each test file.
- Like don't test Channel in the user test file.
That said the above are minor criticisms, mostly on style and organization, not content. You did quite well here. Well done.
| @member_count = member_count | ||
| end | ||
|
|
||
| def print_details |
There was a problem hiding this comment.
Minor suggestion, I would call this method get_details just because it doesn't actually print.
| module SlackCLI | ||
| class SlackApiError < Exception; end | ||
|
|
||
| class Recipient |
| module SlackCLI | ||
| class User < Recipient |
| class Workspace | ||
| attr_reader :users, :channels, :selected, :selected, :query | ||
|
|
||
| Dotenv.load |
There was a problem hiding this comment.
I noticed you have this line in several places, just maybe there is a way to only need to do this once. Otherwise well done.
| def list_all_user | ||
| all_users = [] | ||
| response = HTTParty.get(SlackCLI::User::BASE_URL, query: query)["members"] | ||
|
|
||
| response.each do |person| | ||
| all_users << SlackCLI::User.new(person["name"], person["id"]) | ||
| end | ||
| return all_users | ||
| end |
There was a problem hiding this comment.
Methods like this for user and Channel could be instead class methods in the User and Channel class and that would help separate the command-line stuff in workspace with the HTTP requests in Recipient, Channel and User.
| require_relative "test_helper" | ||
| require_relative "../lib/channel" | ||
|
|
||
| describe "Channel class" do |
There was a problem hiding this comment.
Since a channel is a recipient, you should probably also test sending a message and the print details as well.
| @@ -0,0 +1,35 @@ | |||
| require_relative "test_helper" | |||
There was a problem hiding this comment.
Generally you're never instantiating Recipient, it makes more sense as an abstract class which you would test through Channel and User.
Also I suggest trying to test with minimal dependencies, so testing Recipient without having to use Workspace.
| it "prints detail for given channel" do | ||
| VCR.use_cassette("print_details") do | ||
| # select a channel | ||
| workspace = SlackCLI::Workspace.new | ||
| channel = workspace.select_channel("general") | ||
| expect(channel.print_details).must_be_instance_of Hash | ||
| end | ||
| end |
There was a problem hiding this comment.
You're testing a channel in the user_test.rb file?
| require_relative "test_helper" | ||
| require "simplecov" | ||
|
|
||
| describe "Workspace class" do |
There was a problem hiding this comment.
This has good tests for what Workspace is doing.
2 participants
Assignment Submission: Slack CLI
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection
Question | Answer
1)How did you go about exploring the Slack API? Did you learn anything that would be useful for your next project involving an API? I
The program indicates of an error is "ok" field set to false in JSON. When this happens, my program raises a custom error.
4.) How did the design and organization of your project change over time?
5.) Did you use any of the inheritance idioms we've talked about in class?
6.)How does VCR aid in testing a program that uses an API?