Scissors - Jittania Smith #55
Conversation
…to wave-5-rough sigh
|
I just did a quick refactoring of my instance methods. Didn't affect code functionality but should hopefully look a bit nicer! |
…issions - I'm sorry! I submitted an uncommented version that I thought would be easier to read. No difference in actual code between this version and my last submission.
…to push the changes. No other changes in code were made
…to push the changes. No other changes in code were made
| # has a matching goal id, then returns a JSON dictionary of attribute values | ||
| def convert_to_json(self): | ||
|
|
||
| response_body = { |
| # HELPER FUNCTIONS: | ||
| #============================================================================= | ||
|
|
||
| def slack_post_to_task_notifications_channel(text): |
There was a problem hiding this comment.
Love that you made this its own helper function! That helps make the code easier to read. :)
| requests.post('https://slack.com/api/chat.postMessage', headers=post_headers, data=post_data) | ||
|
|
||
|
|
||
| def valid_id_or_400(input_id): |
| @tasks_bp.route("/<task_id>", methods=["GET"], strict_slashes=False) | ||
| def get_single_task(task_id): | ||
|
|
||
| # ❓ Would it make sense to bundle checking for valid task id and querying a task for that id into one instance method or helper function - since I'm doing that for every endpoint |
There was a problem hiding this comment.
Great question! The question I would ask to answer that question is: "When I call this function, do I always query for a task as well?" And in this code, I see the answer is yes! So I think that would be a great little improvement.
There was a problem hiding this comment.
Okay, cool - The reason I held off is that I think I'm a little fuzzy still on when it makes sense to bundle repeat code like that vs. employing an SRP mentality? I was worried that putting all of those things (checking for input validity, returning 400 if invalid - If valid, querying an instance of something, returning 404 if it can't find it) was too much stuff for a helper function to be doing?
|
|
||
| # This instance method returns a JSON dictionary of attribute values | ||
| # with an optional argument if the goal has tasks assigned to it | ||
| def convert_to_json(self, tasks_list=None): |
There was a problem hiding this comment.
Love this thoughtful, handy optional param here! That said, you shouldn't actually need to ask the caller to pass these in because they are already attached to the goal. See my comment below.
| response_body = { | ||
| "id": self.goal_id, | ||
| "title": self.title | ||
| } | ||
|
|
||
| if tasks_list != None: | ||
| response_body["tasks"] = tasks_list |
There was a problem hiding this comment.
By changing the code to make use of the tasks attribute you added to the Goal (line 8), you guarantee the list of tasks is always exactly what the database says it is, rather than allowing the caller of the function to specify whatever tasks they want to pass in.
Happy to talk more about this if this isn't clear!
(I should note, this will be slightly different behavior that what the current code does because it'll display an empty list for "tasks" when no tasks are present instead of omitting "tasks" from the response.)
| response_body = { | |
| "id": self.goal_id, | |
| "title": self.title | |
| } | |
| if tasks_list != None: | |
| response_body["tasks"] = tasks_list | |
| response_body = { | |
| "id": self.goal_id, | |
| "title": self.title, | |
| "tasks": self.tasks | |
| } |
There was a problem hiding this comment.
That's what I thought - but if I make this change, it causes my Wave 5/6 tests to fail? And I remember trying something like that initially and not being able to get it to work, which is why I went this route. I also had to use tasks = Task.query.filter_by(match_goal_id=goal_id) as opposed to just treating tasks like an attribute of a goal, which was not working. I'm guessing something else in my code needs to be changed as well, but I'm having trouble identifying where. In other words.....yes please, help me to understand!!!!! I'm confused on this one (also you'll probably notice I did a similar thing in my Video Store API because I was having the same issue)
There was a problem hiding this comment.
You're totally right! This suggestion is not enough on its own. Sorry for the confusion and thanks for the thorough response!
There would need to be more code to convert the data retrieved into the appropriate format.
As far as the other confusion about needing to query manually, I'm not sure. I just made a change in your code (I'll add another comment to talk about it) and it passed the tests just fine.
For all of this though, I want to note that these are quite minor comments. The whole discussion we're having here can be fun and interesting to look into but none of it is very important or of high concern!
|
|
||
| db.session.commit() | ||
|
|
||
| return make_response({"id": saved_goal.goal_id, "task_ids": task_ids}, 200) |
There was a problem hiding this comment.
This change would have made the above-mentioned bug more obvious. This would also allow the response to include ALL tasks associated with the goal, not just the new ones.
This isn't super intuitive so I'd definitely be down to talk through this more in our next 1:1
| return make_response({"id": saved_goal.goal_id, "task_ids": task_ids}, 200) | |
| full_task_ids = [] | |
| for task in goal.tasks: | |
| full_task_ids.append(task.task_id) | |
| return make_response({"id": goal.goal_id, "task_ids": full_task_ids}, 200) |
There was a problem hiding this comment.
^ this was the comment I meant!
There was a problem hiding this comment.
Yup! You're right. I didn't actually test out this code and it wouldn't quite work as is
| # filtering the Task query for tasks with a foreign key id that matches the input goal_id | ||
| tasks = Task.query.filter_by(match_goal_id=goal_id) | ||
|
|
||
| for task in tasks: |
There was a problem hiding this comment.
I made the following change in the code and all the tests still pass. I hope that helps with understanding what might have been going wrong that caused whatever you had tried before to not work!
| # filtering the Task query for tasks with a foreign key id that matches the input goal_id | |
| tasks = Task.query.filter_by(match_goal_id=goal_id) | |
| for task in tasks: | |
| for task in saved_goal.tasks: |
There was a problem hiding this comment.
Thank you for taking the time to look at this again! Not being able to get that approach to work was really annoying me, haha. However - Even though that syntax makes sense to me, I'm still having trouble getting the wave 5 and 6 tests to pass, which is kinda what I remember happening when I was writing this (I started off trying to write it the way you show here). I know it's not super urgent - maybe we can just check it out during our next 1:1, I think I'm mostly just curious now. Also - I'm curious how you were able to get the tests to pass given that (in your previous comment, starting in line 296 of app/routes.py) the goal instance is referenced by saved_goal, not goal - such that I would expect goal.tasks to raise a syntax error?
There was a problem hiding this comment.
The code snippet I have here does say saved_goal so I'm confused by the question here, but yes! Let's absolutely talk about this more in our next 1:1 if not before! We're scheduled to meet again next week!
There was a problem hiding this comment.
Yes, that's correct - I'm sorry! it was probably confusing because I wrote the comment here, but was referencing a comment you made on line 296
No description provided.