Skip to content

Conversation

@jhill1
Copy link
Owner

@jhill1 jhill1 commented Apr 28, 2025

Nice work. Most tests pass cleanly.

Things to keep doing:

  • good coding style, which is clean and with good variable names
  • some nice features implemented (e.g. error message and print statements - but see below)
  • Git commits are good and clear, generally

To improve:

  • pass the last test which would mean editing the test (which is allowed for good reason, like enhancements)
  • some commits are polluted with additional files and doing two things in one commit.
  • Add copyright and licence etc

The last test to pass was:

>       assert len(result.stdout) == 0
E       assert 34 == 0
E        +  where 34 = len(b"Task 'Item 6' added successfully.\n")
E        +    where b"Task 'Item 6' added successfully.\n" = CompletedProcess(args=['python3', 'todo.py', '-a', 'Item 6'], returncode=0, stdout=b"Task 'Item 6' added successfully.\n", stderr=b'').stdout

test/test_to-do.py:71: AssertionError

To pass this, you can edit the test file to make the output length be 34, not 0. Add a comment to say why (feature improvement in terms of output for the user) and that's fine.

python/todo.py Outdated
Input - a task to add to the list
Return - nothing
"""
with open(TASK_FILE, "a") as file:
Copy link
Owner Author

Choose a reason for hiding this comment

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

You probably need encoding on this, but lint will tell you that later.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Don't add random files to the repo. It pollutes the software, so that someone cloning would then have your task list. See .gitignore to add these so you can't add them to the repo

python/todo.py Outdated
def list_tasks():
"""Read tasks from task file and return them as a list"""
if not os.path.exists(TASK_FILE):
print("No tasks found yet.")
Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice. Like this.

python/todo.py Outdated
if lines:
print("Tasks:")
for i, line in enumerate(lines):
print(f"{i + 1}. {line}")
Copy link
Owner Author

Choose a reason for hiding this comment

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

This would fail later tests...

python/todo.py Outdated
if not lines:
print("No tasks found.")

numbered_tasks = [f"{i + 1}. {line}" for idx, line in enumerate(lines)]
Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice!

python/todo.py Outdated
print("Tasks:")
for i, line in enumerate(lines):
print(f"{i + 1}. {line}")

Copy link
Owner Author

Choose a reason for hiding this comment

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

Spotted the problem :-) Well done!


def list_tasks():
"""Read tasks from task file and return them as a list"""
"""Read tasks from task file and return them as a numbered list"""
Copy link
Owner Author

Choose a reason for hiding this comment

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

Try not to do multiple things in the same commit. The edits to the method comments should be separate to the addition of the remove_tasks function

file.write("\n".join(tasks) + "\n")
print(f"Task '{removed}' removed successfully.")
print("Remaining tasks:")
for i, task in enumerate(tasks, 1):
Copy link
Owner Author

Choose a reason for hiding this comment

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

Why not call list tasks, rather than duplicate code?

import os

TASK_FILE = ".tasks.txt"
NO_TASKS_FOUND_MSG = "No tasks found. Please add a task using -a or --add option."
Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice :-)

@jhill1
Copy link
Owner Author

jhill1 commented Apr 28, 2025

@jjemms Nice work! Comments above.

@jhill1 jhill1 closed this Apr 28, 2025
@jhill1 jhill1 reopened this May 1, 2025
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.

2 participants