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

Show file tree
Hide file tree
Changes from 3 commits
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
101 changes: 82 additions & 19 deletions moocfi_cses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
from getpass import getpass
import logging
import json
import urllib.parse
from urllib.parse import urljoin
from pathlib import Path
from typing import AnyStr, Optional
from xml.etree.ElementTree import Element, tostring
Comment on lines +13 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider consolidating imports from the same module.

- from urllib.parse import urljoin
- from pathlib import Path
- from typing import AnyStr, Optional
+ from urllib.parse import urljoin
+ from pathlib import Path
+ from typing import AnyStr, Optional
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
from urllib.parse import urljoin
from pathlib import Path
from typing import AnyStr, Optional
from xml.etree.ElementTree import Element, tostring
from urllib.parse import urljoin
from pathlib import Path
from typing import AnyStr, Optional
from xml.etree.ElementTree import Element, tostring


from html2text import html2text
import htmlement
import requests

Expand All @@ -34,7 +36,7 @@ def __post_init__(self) -> None:

@property
def is_logged_in(self) -> bool:
html = self.http_request(self.base_url)
html = self.http_request(urljoin(self.base_url, "list"))
login_link = find_link(html, './/a[@class="account"]')
login_text = login_link.get("text") or ""
return self.username in login_text
Expand All @@ -53,10 +55,10 @@ def login(self) -> None:
if self.is_logged_in:
return

res = self.http_session.get(self.base_url)
res = self.http_session.get(urljoin(self.base_url, "list"))
login_link = find_link(res.text, './/a[@class="account"]')
if login_link:
login_url = urllib.parse.urljoin(res.url, login_link.get("href"))
login_url = urljoin(res.url, login_link.get("href"))
else:
logging.debug(
f"url: {res.url}, status: {res.status_code}\nhtml:\n{res.text}"
Expand All @@ -78,7 +80,7 @@ def login(self) -> None:
login_form["session[password]"] = self.password

self.http_session.post(
url=urllib.parse.urljoin(res.url, action),
url=urljoin(res.url, action),
headers={"referer": res.url},
data=login_form,
)
Expand Down Expand Up @@ -130,16 +132,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")
Comment on lines +135 to +154
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistency in the help messages and argument choices for subparsers.

parser_login = subparsers.add_parser("login", help="Login to mooc.fi CSES")
parser_list = subparsers.add_parser("list", help="List exercises")
parser_show = subparsers.add_parser("show", help="Show details of an exercise")


return parser.parse_args(args)

Expand Down Expand Up @@ -252,6 +264,9 @@ class Task:
id: str
name: str
state: TaskState
description: str = "N/A"
code: str = "N/A"
submit_file: str = "N/A"


# TODO: this should be part of a client class or module
Expand Down Expand Up @@ -288,10 +303,59 @@ 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
# TODO: should we split up this function in a bunch of smaller ones? or will beautifulsoup make it simpler?
def parse_task(html: str | bytes) -> Task:
root = htmlement.fromstring(html)
task_link_element = root.find('.//div[@class="nav sidebar"]/a')
task_link = task_link_element if task_link_element is not None else Element("a")
task_id = task_link.get("href", "").split("/")[-1]
task_name = task_link.text or "N/A"
task_span_element = task_link.find("span")
task_span = task_span_element if task_span_element is not None else Element("span")
task_span_class = task_span.get("class", "")
desc_div_element = root.find('.//div[@class="md"]')
desc_div = desc_div_element if desc_div_element is not None else Element("div")
description = html2text(tostring(desc_div).decode("utf8"))
code = root.findtext(".//pre", "N/A")
submit_file = next(
iter(
[
code_element.text
for code_element in root.findall(".//code")
if code_element.text is not None and ".py" in code_element.text
]
),
"N/A",
)
task = Task(
id=task_id,
name=task_name,
state=TaskState.COMPLETE if "full" in task_span_class else TaskState.INCOMPLETE,
description=description.strip(),
code=code,
submit_file=submit_file,
)

return task
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.

Tip

Codebase Verification

The parse_task function in moocfi_cses.py does not include exception handling mechanisms. This could lead to unhandled errors if unexpected input or conditions are encountered during its execution. It is recommended to implement try-except blocks to manage potential exceptions effectively, especially given the function's interaction with external data (HTML parsing and attribute access).

  • Consider wrapping sections of the code that parse HTML and access attributes in try-except blocks to handle possible exceptions like AttributeError or parsing errors.
  • It may also be beneficial to validate the existence and correctness of the HTML structure before attempting to parse it.
Analysis chain

Ensure that parse_task handles exceptions and edge cases effectively.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `parse_task` handles exceptions and edge cases effectively.

# Test: Search for exception handling in `parse_task`.
rg --type python $'try:|except'

Length of output: 63

Script:

#!/bin/bash
# Description: Verify that `parse_task` handles exceptions and edge cases effectively.

# Corrected Test: Search for exception handling in `parse_task`.
rg --type py $'try:|except'

Length of output: 276

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 to address the lack of exception handling in the parse_task function of the moocfi_cses.py file. You can view and track the issue here. If there's anything else you need, feel free to ask!

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a GitHub issue regarding the lack of exception handling in the `parse_task` function.

gh issue create \
  --title "Add Exception Handling to parse_task Function" \
  --body "The `parse_task` function in `moocfi_cses.py` currently lacks proper exception handling mechanisms. This could lead to unhandled errors if unexpected input or conditions are encountered during its execution, especially given the function's interaction with external data (HTML parsing and attribute access). It is recommended to implement try-except blocks to manage potential exceptions effectively, and to validate the existence and correctness of the HTML structure before attempting to parse it. This issue was identified in the context of PR https://github.com/madeddie/moocfi_cses/pull/24." \
  --repo madeddie/moocfi_cses

Length of output: 923



def print_task(task: Task) -> None:
print(f"{task.id}: {task.name} {TASK_STATE_ICON[task.state]}")
print(task.description)
print(f"\nSubmission file name: {task.submit_file}")


# TODO: Implement function that posts the submit form with the correct file
Expand All @@ -301,12 +365,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 +374,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 All @@ -326,7 +384,7 @@ def main() -> None:
# Merge cli args and configfile parameters in one dict
config.update((k, v) for k, v in vars(args).items() if v is not None)

base_url = f"https://cses.fi/{config['course']}/list/"
base_url = f"https://cses.fi/{config['course']}/"

cookiefile = None
cookies = dict()
Expand All @@ -350,9 +408,14 @@ def main() -> None:
write_cookie_file(str(cookiefile), cookies)

if args.cmd == "list":
html = session.http_request(base_url)
html = session.http_request(urljoin(base_url, "list"))
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 args.cmd == "show":
html = session.http_request(urljoin(base_url, f"task/{args.task_id}"))
task = parse_task(html)
print_task(task)


if __name__ == "__main__":
Expand Down
5 changes: 2 additions & 3 deletions tests/test_moocfi_cses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ class TestParseForm:
)


# TODO: verify these mocked tests are actually accurate
# TODO: add tests for unreachable and failing endpoints, 4xx, 5xx, etc
@pytest.fixture
def mock_session() -> moocfi_cses.Session:
Expand All @@ -64,7 +63,7 @@ def test_login_successful(mock_session: moocfi_cses.Session) -> None:
# Mocking the HTTP response for successful login
with requests_mock.Mocker() as m:
m.get(
"https://example.com",
"https://example.com/list",
text='<a class="account" href="/user/1234">[email protected] (mooc.fi)</a>',
)
mock_session.login()
Expand All @@ -75,7 +74,7 @@ def test_login_failed(mock_session: moocfi_cses.Session) -> None:
# Mocking the HTTP response for failed login
with requests_mock.Mocker() as m:
m.get(
"https://example.com",
"https://example.com/list",
text='<a class="account" href="/login/oauth-redirect?site=mooc.fi">Login using mooc.fi</a>',
)
m.get("https://example.com/account", text="Login required")
Expand Down