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

Moved TODOs into github issues #32

Merged
merged 1 commit into from
May 13, 2024
Merged

Moved TODOs into github issues #32

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 1 addition & 19 deletions moocfi_cses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
# TODO: if config doesn't exist fail and ask to run config creation
# TODO: make sure the correct directories exist
# TODO: check validity of config after creation (can we log in?)
# TODO: add exercise list parse, list of exercises, name, status, possible: week and deadline
# TODO: UI for checking exercise description
# TODO: UI for submitting solutions
import argparse
from dataclasses import dataclass, field
from enum import Enum
Comment on lines 1 to 3
Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

Your changes indicate the removal of several TODO comments and the import of argparse, dataclasses, and enum. It seems like you're preparing for some argument parsing and structured data handling. A few suggestions:

  1. Comment Removal: If these TODOs were removed because they've been implemented, consider ensuring there's clear documentation or comments in place where each task was addressed.
  2. Data Handling: Using dataclass and Enum is a good choice for clean and efficient data representation. Ensure each dataclass and Enum is well-defined and includes type hints for clarity.

Example:

from enum import Enum
from dataclasses import dataclass

class Status(Enum):
    TODO = "TODO"
    IN_PROGRESS = "IN_PROGRESS"
    DONE = "DONE"

@dataclass
class Exercise:
    name: str
    status: Status
    week: int = field(default=0)
    deadline: str = field(default="")
  1. Argument Parsing: It's great you're setting up for command-line interaction. Make sure to define clear, user-friendly arguments with argparse, including help descriptions for each argument.

Remember, well-commented code and clear structure are key to maintainability and overall project health.

Expand Down Expand Up @@ -41,7 +35,6 @@ def is_logged_in(self) -> bool:
login_text = login_link.get("text") or ""
return self.username in login_text

# TODO: create custom exceptions
def login(self) -> None:
"""Logs into the site using webscraping

Comment on lines 35 to 40
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 removal of the comment "TODO: create custom exceptions" suggests you might have addressed custom exception handling elsewhere. However, it's crucial to ensure that exception handling, especially custom exceptions, is implemented for robust error management. Custom exceptions could improve clarity and maintainability.

Consider defining exceptions like LoginError:

class LoginError(Exception):
    """Exception raised for errors during login."""
    pass

And using it in your login method:

def login(self) -> None:
    try:
        # login logic here
        pass
    except Exception as e:
        raise LoginError("Failed to log in") from e

Expand Down Expand Up @@ -107,7 +100,6 @@ def http_request(
return res.text


# TODO: replace with click
def parse_args(args: Optional[list[str]] = None) -> argparse.Namespace:
parser = argparse.ArgumentParser(description="Interact with mooc.fi CSES instance")
parser.add_argument("--username", help="tmc.mooc.fi username")
Comment on lines 100 to 105
Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

This change suggests an intent to replace argparse with click but hasn't been implemented yet. While argparse is a robust option for command-line interface creation, click can offer a more straightforward syntax and automatic help page generation. Since the code changes are minimal and the transition to click isn't shown, I recommend:

  1. Implement click to see the benefits of cleaner syntax and easier command line options handling.

Example with click:

import click

@click.command()
@click.option('--username', help='tmc.mooc.fi username')
def main(username):
    click.echo(f"Hello, {username}")
  1. Complete the TODO before merging.

Expand Down Expand Up @@ -157,7 +149,6 @@ def parse_args(args: Optional[list[str]] = None) -> argparse.Namespace:


def create_config() -> dict[str, str]:
# TODO: try to read an existing config file and give the values as default values
username = input("Your tmc.mooc.fi username: ")
password = getpass("Your tmc.mooc.fi password: ")
config = {
Comment on lines 149 to 154
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 TODO comment suggests implementing functionality to read an existing config file for default values. It's crucial to provide a fallback or initialization from existing configurations to enhance user experience and prevent unnecessary reconfigurations.

Consider using a function to attempt reading an existing config file, and if it exists, load these values as defaults:

import json

def read_existing_config() -> dict[str, str]:
    try:
        with open('config.json', 'r') as file:
            return json.load(file)
    except FileNotFoundError:
        return {}

def create_config() -> dict[str, str]:
    existing_config = read_existing_config()
    username = input("Your tmc.mooc.fi username: ") or existing_config.get('username', '')
    password = getpass("Your tmc.mooc.fi password: ") or existing_config.get('password', '')
    config = {

This approach improves the function by providing a smoother setup or re-setup experience.

Expand All @@ -168,21 +159,17 @@ def create_config() -> dict[str, str]:
return config


# TODO: check if file exists and ask permission to overwrite
def write_config(configfile: str, config: dict[str, str]) -> None:
file_path = Path(configfile).expanduser()
if file_path.exists():
# TODO: check if file exists and ask permission to overwrite
# Prompt user or handle file overwrite scenario
# TODO: https://github.com/madeddie/moocfi_cses/issues/28
...
file_path.parent.mkdir(parents=True, exist_ok=True) # Ensure directory exists
print("Writing config to file")
with open(file_path, "w") as f:
json.dump(config, f)


# TODO: check if path exists
# TODO: try/except around open and json.load, return empty dict on failure
def read_config(configfile: str) -> dict[str, str]:
config = dict()
file_path = Path(configfile).expanduser()
Comment on lines 159 to 175
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 replacing specific TODO comments with a link to an issue is a good practice for tracking, but ensure the issue contains clear instructions or code examples on the solution. For the write_config function, handling file overwrite by asking for user permission is crucial for user experience and preventing accidental data loss.

Instead of a placeholder or generic comments, consider implementing an interactive prompt or a mechanism to backup before overwriting, e.g.:

from shutil import copy

if file_path.exists():
    overwrite = input(f"{file_path} exists. Overwrite? (y/n): ").strip().lower()
    if overwrite == 'y':
        backup_path = f"{file_path}.bak"
        copy(file_path, backup_path)
        print(f"Backup created at {backup_path}")
    else:
        print("Operation cancelled.")
        return

For read_config, indeed adding try/except around file opening and loading is important for robustness:

try:
    with open(file_path, 'r') as f:
        config = json.load(f)
except (FileNotFoundError, json.JSONDecodeError):
    return {}

Expand Down Expand Up @@ -269,7 +256,6 @@ class Task:
submit_file: str = "N/A"


# TODO: this should be part of a client class or module
def parse_task_list(html: AnyStr) -> list[Task]:
"""Parse html to find tasks and their status, return something useful, possibly a specific data class"""
content_element = htmlement.fromstring(html).find('.//div[@class="content"]')
Comment on lines 256 to 261
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 code correctly identifies the need for organizing the parse_task_list function into a client class or module for better structure. This change encapsulates functionality and could enable easy maintenance and scalability.

However, ensure AnyStr from typing is used appropriately to accept both str and bytes types. If only one is expected, use str or bytes instead for clarity.

Consider introducing a data class for Task to ensure the returned list has a well-defined structure, improving code readability and safety.

Example improvement:

from typing import List
from dataclasses import dataclass

@dataclass
class Task:
    # Assuming tasks attributes here
    id: int
    status: str

class TaskParser:
    @staticmethod
    def parse_task_list(html: str) -> List[Task]:
        content_element = htmlement.fromstring(html).find('.//div[@class="content"]')
        # Parsing logic here
        return [Task(id=1, status='Complete')]  # Example return

Expand Down Expand Up @@ -302,7 +288,6 @@ def parse_task_list(html: AnyStr) -> list[Task]:
return task_list


# TODO: This should be part of a UI class or module
def print_task_list(
task_list: list[Task], filter: Optional[str] = None, limit: Optional[int] = None
) -> None:
Comment on lines 288 to 293
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 refactoring into a UI class or module is a great idea for code organization and readability. However, consider implementing type hinting for list[Task] more explicitly if Task is a custom class. Use from typing import List and define the type of elements within the list for improved clarity.

from typing import List, Optional
from your_project.task import Task  # Assuming Task is a custom class

class UITaskManager:
    @staticmethod
    def print_task_list(
        task_list: List[Task], filter: Optional[str] = None, limit: Optional[int] = None
    ) -> None:
        # Implementation here

This change facilitates readability and future maintenance by clarifying that task_list is a list of Task instances.

Expand All @@ -315,8 +300,6 @@ def print_task_list(
return


# TODO: Implement function that parser the specific task page into Task object
# TODO: should we split up this function in a bunch of smaller ones? or will beautifulsoup make it simpler?
def parse_task(html: AnyStr) -> Task:
root = htmlement.fromstring(html)
task_link_element = root.find('.//div[@class="nav sidebar"]/a')
Comment on lines 300 to 305
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 implementation of parse_task is a good start. However, there are a few points to consider:

  1. The removal of TODO comments without apparent implementation hints at unfinished work or missing functionality. Ensure all intended features are implemented or tracked elsewhere.

  2. For readability and future maintenance, adding type annotations for the return type of htmlement.fromstring(html) would be helpful, assuming root has a specific type that could be annotated.

  3. It might be beneficial to consider error handling if .find does not locate the element, preventing potential runtime errors.

Here's a potential improvement:

from typing import Optional  # If not already imported

def parse_task(html: AnyStr) -> Optional[Task]:
    try:
        root = htmlement.fromstring(html)  # Assuming root's type can be more specifically annotated
        task_link_element = root.find('.//div[@class="nav sidebar"]/a')
        if not task_link_element:
            return None  # Or handle absence in another way
        # Process task_link_element into a Task object...
    except Exception as e:
        # Properly handle or log the exception
        return None

This adjustment adds robustness against potential parsing failures and clarifies the handling of cases where elements are not found.

Expand Down Expand Up @@ -358,7 +341,6 @@ def print_task(task: Task) -> None:
print(f"\nSubmission file name: {task.submit_file}")


# TODO: Implement function that posts the submit form with the correct file
def submit_task(task_id: str, filename: str) -> None:
"""submit file to the submit form or task_id"""
# NOTE: use parse_form
Comment on lines 341 to 346
Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

It seems you intend to implement a feature to submit tasks. A few suggestions to improve the current changes:

  1. Docstring Improvement: Ensure your docstring accurately describes the function, including its parameters and what it returns or accomplishes. Since the function is not yet implemented, consider noting that it's a work in progress.

  2. Use of TODOs: It's good you've noted the need to implement the function but ensure in subsequent commits the implementation follows soon after for maintainability.

  3. Function Implementation Placeholder: If the function is incomplete, perhaps raise a NotImplementedError as a placeholder to avoid silent failures if the function is mistakenly called.

Example:

def submit_task(task_id: str, filename: str) -> None:
    """
    Submit file to the submit form for a given task_id.
    
    Parameters:
    - task_id: The ID of the task to submit to.
    - filename: The name of the file to submit.
    
    Returns:
    None. Submits the task and handles the submission process.
    """
    raise NotImplementedError("Function submit_task is not yet implemented.")

Expand Down