Moved TODOs into github issues #32
Conversation
WalkthroughThe recent updates to the codebase reflect a shift towards enhanced functionality and cleaner code. Major improvements include the removal of outdated Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![senior-dev-bot[bot]](https://avatars.githubusercontent.com/in/282840?s=60&v=4){kind=small}
| 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 | ||
|
|
There was a problem hiding this comment.
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."""
passAnd 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| 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: |
There was a problem hiding this comment.
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 hereThis change facilitates readability and future maintenance by clarifying that task_list is a list of Task instances.
|
|
||
|
|
||
| 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 = { |
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
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:
- Implement
clickto 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}")- Complete the TODO before merging.
| 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() |
There was a problem hiding this comment.
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.")
returnFor 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 {}| 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 |
There was a problem hiding this comment.
CODE REVIEW
It seems you intend to implement a feature to submit tasks. A few suggestions to improve the current changes:
-
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.
-
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.
-
Function Implementation Placeholder: If the function is incomplete, perhaps raise a
NotImplementedErroras 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.")| import argparse | ||
| from dataclasses import dataclass, field | ||
| from enum import Enum |
There was a problem hiding this comment.
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:
- 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.
- Data Handling: Using
dataclassandEnumis a good choice for clean and efficient data representation. Ensure eachdataclassandEnumis 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="")- 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.
| 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') |
There was a problem hiding this comment.
CODE REVIEW
The implementation of parse_task is a good start. However, there are a few points to consider:
-
The removal of TODO comments without apparent implementation hints at unfinished work or missing functionality. Ensure all intended features are implemented or tracked elsewhere.
-
For readability and future maintenance, adding type annotations for the return type of
htmlement.fromstring(html)would be helpful, assumingroothas a specific type that could be annotated. -
It might be beneficial to consider error handling if
.finddoes 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 NoneThis adjustment adds robustness against potential parsing failures and clarifies the handling of cases where elements are not found.
| 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"]') |
There was a problem hiding this comment.
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
TODOcomments and restructuring functions.