Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rename some subcommands in line with tmc-cli #24

Merged
merged 4 commits into from
May 13, 2024
Merged

Rename some subcommands in line with tmc-cli #24

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
760dcc9
Rename some subcommands in line with tmc-clie
madeddie May 13, 2024
106003e
Add the show task functions
madeddie May 13, 2024
83a9c0e
Fix tests for refactored base_url
madeddie May 13, 2024
14faf25
Use AnyStr instead of str | bytes
madeddie May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions moocfi_cses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,16 +130,26 @@ def parse_args(args: Optional[list[str]] = None) -> argparse.Namespace:
)
subparsers = parser.add_subparsers(required=True)

parser_config = subparsers.add_parser("configure", help="Configure moocfi_cses")
parser_config.set_defaults(cmd="configure")
# login subparser
parser_login = subparsers.add_parser("login", help="Login to mooc.fi CSES")
parser_login.set_defaults(cmd="login")

# list exercises subparser
parser_list = subparsers.add_parser("list", help="List exercises")
parser_list.set_defaults(cmd="list")
parser_list.add_argument(
"--filter",
help="List complete, incomplete or all tasks (default: %(default)s)",
help="List only complete or incomplete tasks (default: all)",
choices=["complete", "incomplete"],
)
parser_list.add_argument(
"--limit", help="Maximum amount of items to list", type=int
)

# show exercise subparser
parser_show = subparsers.add_parser("show", help="Show details of an exercise")
parser_show.set_defaults(cmd="show")
parser_show.add_argument("task_id", help="Numerical task identifier or task name")

return parser.parse_args(args)

Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

Changes are well-directed towards enhancing the CLI tool's functionality by introducing new commands (login, list updates, show). However, for maintainability and scalability:

  1. Consistency in Help Messages:
    Ensure help messages maintain a consistent style in terms of sentence structure and capitalization. For example, "Login to mooc.fi CSES" starts with a capital letter, whereas others do not.

  2. Argument Choices for --filter:
    The update to --filter argument's help message is good but limiting choices to "complete" or "incomplete" and defaulting to "all" can be confusing since "all" is not a selectable option. Consider including "all" in the choices or adjust the implementation to handle its default behavior more intuitively.

  3. Extensibility of Argument Adding:
    For adding arguments to subparsers, consider creating a more modular approach if the subparsers and their arguments start to grow. This could involve defining a structure or configuration for commands and their arguments in a separate place and adding them in a loop, improving code readability and ease of updates.

    Example for a more consistent help message and including "all" in choices:

    parser_list.add_argument(
    "--filter",
    help="List only complete, incomplete, or all tasks (default: all)",
    choices=["complete", "incomplete", "all"],
)

Expand Down Expand Up @@ -288,10 +298,22 @@ def parse_task_list(html: str | bytes) -> list[Task]:


# TODO: This should be part of a UI class or module
def print_task_list(task_list: list[Task], filter: Optional[str] = None) -> None:
def print_task_list(
task_list: list[Task], filter: Optional[str] = None, limit: Optional[int] = None
) -> None:
count: int = 0
for task in task_list:
if not filter or filter == task.state.value:
print(f"- {task.id}: {task.name} {TASK_STATE_ICON[task.state]}")
count += 1
if limit and count >= limit:
return

Comment on lines +306 to +316
Copy link
Contributor

@coderabbitai coderabbitai bot May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Consider refactoring print_task_list to separate filtering and limiting logic into different functions.

def print_filtered_tasks(task_list: list[Task], filter: Optional[str] = None) -> list[Task]:
    return [task for task in task_list if not filter or filter == task.state.value]

def print_limited_tasks(filtered_tasks: list[Task], limit: Optional[int] = None) -> None:
    for count, task in enumerate(filtered_tasks, 1):
        print(f"- {task.id}: {task.name} {TASK_STATE_ICON[task.state]}")
        if limit and count >= limit:
            break

Copy link
Owner Author

Choose a reason for hiding this comment

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

@coderabbitai make a github issue for this please

Copy link
Contributor

Choose a reason for hiding this comment

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

@madeddie, the GitHub issue has been successfully created for refactoring the print_task_list function to separate the filtering and limiting logic. You can view and track the issue here.

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a GitHub issue for refactoring the print_task_list function
gh issue create \
  --title "Refactor print_task_list to separate filtering and limiting logic" \
  --body "It's suggested to refactor the `print_task_list` function to separate the filtering and limiting logic into different functions for better readability and maintainability. This change can help in managing the tasks more efficiently and make the codebase cleaner.

Related PR: https://github.com/madeddie/moocfi_cses/pull/24
Comment: https://github.com/madeddie/moocfi_cses/pull/24#discussion_r1598756440" \
  --repo madeddie/moocfi_cses

Length of output: 672


# TODO: Implement function that parser the specific task page into Task object
def parse_task(html: str | bytes, task: Task) -> Task:
task = Task("a", "b", TaskState.COMPLETE)
return task
Copy link
Contributor

Choose a reason for hiding this comment

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

The function parse_task appears to be incomplete, returning a hardcoded Task object.

Would you like assistance in implementing this function, or should I open a GitHub issue to track this task?



# TODO: Implement function that posts the submit form with the correct file
Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

  • For print_task_list, now that it has more responsibilities, consider refactoring it for single responsibility. Extract filtering and limiting logic into separate functions or within a class structure. For scalability and maintainability, this keeps functions more focused and simpler to manage.
def print_filtered_tasks(task_list: list[Task], filter: Optional[str] = None) -> list[Task]:
    ...
def print_limited_tasks(task_list: list[Task], limit: int) -> None:
    ...

  • The temporary default parameters ("a", "b") in parse_task should be replaced with actual logic for parsing HTML.

  • It's better to explicitly import or define what Task, TaskState, and TASK_STATE_ICON are to avoid confusion.

  • Consider adding type hints for the return type of parse_task.

  • Lastly, add more descriptive TODOs or implement the features to avoid ambiguity.

Expand All @@ -301,12 +323,6 @@ def submit_task(task_id: str, filename: str) -> None:
...


# TODO: Implement function that parser the specific task page into Task object
def parse_task(html: str | bytes, task: Task) -> Task:
task = Task("a", "b", TaskState.COMPLETE)
return task


def main() -> None:
args = parse_args()

Comment on lines 365 to 370
Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

The removed parse_task function was marked as a TODO, suggesting it was placeholder code. However, removing this function entirely without implementing or replacing its functionality might leave a gap in the application's capabilities, especially if other parts of the code depend on it. If the function's logic has been moved elsewhere or is no longer needed, ensure that this is documented or communicated to the team. If this function is to be implemented later, consider using clearer documentation or creating a ticket in your project tracking system to ensure it does not get forgotten.

# If the function is to be implemented later, consider keeping a placeholder or comment indicating future work
# TODO: Implement function that parses the specific task page into Task object
# Placeholder for future implementation

Expand All @@ -316,7 +332,7 @@ def main() -> None:
datefmt="%Y-%m-%d %H:%M:%S",
)

if args.cmd == "configure":
if args.cmd == "login":
config = create_config()
write_config(args.config, config)
return
Comment on lines 374 to 380
Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

The change from "configure" to "login" in the conditional statement is clear. Ensure the rest of your application aligns with this new command naming to maintain consistency. Additionally, verify that all documentation and help messages reflect this change to avoid confusion for users.

Consider providing feedback or a success message to the user upon successful login or configuration update, enhancing the usability of your application.

if args.cmd == "login":
    config = create_config()
    write_config(args.config, config)
    print("Login successful. Configuration saved.")
    return

Expand Down Expand Up @@ -352,7 +368,7 @@ def main() -> None:
if args.cmd == "list":
html = session.http_request(base_url)
task_list = parse_task_list(html)
print_task_list(task_list, filter=args.filter)
print_task_list(task_list, filter=args.filter, limit=args.limit)


if __name__ == "__main__":
Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

The change properly extends the functionality of print_task_list by adding a limit parameter based on args.limit, maintaining readability and clear function usage. It's a practical update if print_task_list is already modified to handle this new limit parameter correctly. However, ensure args.limit is validated before use, especially for being a non-negative integer, to avoid runtime errors.

# Ensure args.limit is validated earlier in the code
if args.limit is not None and (not args.limit.isdigit() or int(args.limit) < 0):
    raise ValueError("Limit must be a non-negative integer.")

Expand Down