Task List, C17 Sea Turtles, Shelby Faulconer

[pickled-bot]

:) yay, Task List

[kelsey-steven-ada]

Great work pulling together many concepts in this project Shelby! Check out the feedback when you have time and reach out if you have questions or need clarifications on any of the feedback 😊

[kelsey-steven-ada]

It looks like the import desc isn't used in goal_routes.py, so we should remove it as part of our code cleanup and refactor steps.

[kelsey-steven-ada]

Since we don't perform actions with response_body before returning it, I'd recommend returning on the same line rather than creating a variable. If the goal is to break things up for readability, I'd consider breaking out the message to a variable:

response_body = {"details": "Invalid data"}
return make_response(jsonify(response_body), 400)

We might also want to give the user more information to help them debug. It could be as little as adding what key was missing:

response_body = {"details": "Invalid data: missing 'title'"}

[kelsey-steven-ada]

To be consistent with whitespace inside functions, I recommend removing one of the new lines both here and above goal.

[kelsey-steven-ada]

One of the things to look at when we are in our code clean up phase is whitespace across a file. The general guideline is one newline between functions inside of a class, but what's most important is to be consistent across a codebase. A little extra whitespace is often used as a visual separation between groups of related functions, or to make it clear where imports end and implementation code begins. We lose the ability to have a visual separation have meaning if we are not consistent with how they are applied. The PEP 8 style guide has a little more info on their recommendations: https://peps.python.org/pep-0008/#blank-lines

[kelsey-steven-ada]

We want to make sure to delete commented out code as part of clean up. When we are sharing a codebase with other folks, it's unclear what the intention of commented out code is, especially without extra comments explaining why it's there. We use versioning tools like git so we can confidently delete code and view our commit history to recover code if we ever need to revert back to something we had prior.

[kelsey-steven-ada]

To me, manage_post_goals doesn't feel very specific about what work this function performs. What's a name that reflects the effects of the function, so someone wouldn't need to read the function internals to know what it does?

[kelsey-steven-ada]

Nice handling for partial updates!

What you have here is what I did in roundtable, but we could omit the line task_keys = request_body.keys() if we wanted. The syntax <string> in <dict> by default checks the dictionary keys, so this could also be written:

    if "title" in request_body:
        task.title = request_body["title"]
    if "description" in request_body:
        task.description = request_body["description"]

[kelsey-steven-ada]

To be consistent with the PEP 8 style guide recommendations for line length, we would want to split this across a couple lines. One approach would be to separate out the dictionary to a variable then pass that variable into jsonify on this line.

[kelsey-steven-ada]

I would consider some gentle reorganization and changing of whitespace to group together the related operations in this function

    # Update task in DB
    task = get_task(id)
    task.completed_at = datetime.date.today()
    db.session.commit()
    
    # Gather info for Slack API call and send POST
    text = f"Someone just completed the task {task.title}"
    channel_id = "C03ENLYJ7AT"
    path = f"https://slack.com/api/chat.postMessage?channel={channel_id}&text={text}"
    headers = {"Authorization": api_key}
    requests.post(path, headers=headers)

    # Return the updated task info
    return {"task":task.to_dictionary()}

[kelsey-steven-ada]

We would want to remove most of the whitespace in this function since it's separating lines that are tightly related to each other. This applies to other functions across the project with similar spacing. An example of what this could look like is:

    task = get_task(id)
    task.completed_at = None
    db.session.commit()

    return {"task":task.to_dictionary()}

[kelsey-steven-ada]

Since api_key is only used in update_task_mark_complete_by_id we would want to declare it inside that function rather than giving it global scope where all functions have access to it.