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

Implement submit action #33

Merged
merged 6 commits into from
May 20, 2024
Merged

Implement submit action #33

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e38a5a
First version of the submit task code
madeddie May 14, 2024
a595f44
First version of the submit task code
madeddie May 14, 2024
e555cc0
Run ruff and black on code
madeddie May 16, 2024
ee1428b
Merge branch '27-implement-exercise-solution-submission-submit-subcom…
madeddie May 20, 2024
b2af18f
Fix code and clean up
madeddie May 20, 2024
bd13201
Split off some functionality into its own function
madeddie May 20, 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
80 changes: 72 additions & 8 deletions moocfi_cses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import json
from urllib.parse import urljoin
from pathlib import Path
from time import sleep
from typing import AnyStr, Optional
from xml.etree.ElementTree import Element, tostring

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 changes introduce the sleep function from the time module. Ensure that using sleep in your code is necessary, as it can introduce delays that might not be desirable, especially in IO-bound or async applications. If its use is unavoidable for rate-limiting or similar purposes, consider alternatives like async sleep functionalities in asynchronous environments or other throttling mechanisms.

No further improvements or issues are noticeable based solely on the import statements. However, always consider the impact of adding new dependencies or modules on the application's performance and maintainability.

Expand Down Expand Up @@ -145,6 +146,12 @@ def parse_args(args: Optional[list[str]] = None) -> argparse.Namespace:
parser_show.set_defaults(cmd="show")
parser_show.add_argument("task_id", help="Numerical task identifier")

# submit exercise solution subparser
parser_submit = subparsers.add_parser("submit", help="Submit an exercise solution")
parser_submit.set_defaults(cmd="submit")
parser_submit.add_argument("--filename", help="Filename of the solution to submit")
parser_submit.add_argument("task_id", help="Numerical task identifier")
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify required=True for the --filename argument to enhance usability.

parser_submit.add_argument("--filename", required=True, help="Filename of the solution to submit")


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

It's generally clean and modular. However, specifying the task_id as a positional argument in both subparsers could imply inconsistency if these actions (show and submit) fundamentally operate on tasks in different ways. If both actions require task_id similarly, it's alright. Otherwise, consider adding more explicit argument types for clarity, e.g., using type=int to enforce numerical IDs. Additionally, adding required=True to required arguments enhances usability.

Example:

parser_submit.add_argument("--filename", required=True, help="Filename of the solution to submit")
parser_submit.add_argument("task_id", type=int, help="Numerical task identifier")

Expand Down Expand Up @@ -221,7 +228,7 @@ def find_link(html: AnyStr, xpath: str) -> dict[str, str | None]:
return link_data


def parse_form(html: AnyStr, xpath: str = ".//form") -> dict[str, str | None]:
def parse_form(html: AnyStr, xpath: str = ".//form") -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify the value types in the return type annotation for clarity and type safety.

def parse_form(html: AnyStr, xpath: str = ".//form") -> dict[str, Optional[str]]:

"""Search for the first form in html and return dict with action and all other found inputs"""
form_element = htmlement.fromstring(html).find(xpath)
form_data = dict()
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's generally good practice to be explicit about types in Python, especially with type hinting in Python 3. It improves code readability and helps with debugging and maintaining. The change here removes the specificity of the return type for parse_form, making it less informative. Consider keeping or even expanding the type hints for better clarity and utility.

Before:

def parse_form(html: AnyStr, xpath: str = ".//form") -> dict:

After:

from typing import Union, Optional, Dict

def parse_form(html: AnyStr, xpath: str = ".//form") -> Dict[str, Optional[str]]:

Expand Down Expand Up @@ -251,9 +258,10 @@ class Task:
id: str
name: str
state: TaskState
description: str = "N/A"
code: str = "N/A"
submit_file: str = "N/A"
description: Optional[str] = None
code: Optional[str] = None
submit_file: Optional[str] = None
submit_link: Optional[str] = None


def parse_task_list(html: AnyStr) -> list[Task]:
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 default string values to Optional[str] with defaults of None is a good approach for clarifying potentially unset values. However, it's essential to consider how this affects other parts of the code that may expect string values. For instance, if there are operations like string concatenation or formatting with these variables, those would now potentially raise exceptions if not checked for None.

# Original
description: str = "N/A"

# Updated
description: Optional[str] = None

Ensure any function handling Task objects is prepared to handle these None values appropriately.

Expand Down Expand Up @@ -313,6 +321,13 @@ def parse_task(html: AnyStr) -> Task:
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_link_element = root.find('.//a[.="Submit"]')
submit_link = (
submit_link_element.get("href", "N/A")
if submit_link_element is not None
else "N/A"
)

submit_file = next(
iter(
[
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 changes generally look good, but you could improve readability and consistency by adopting the same pattern as your initial condition with desc_div_element. Consider using a ternary operator immediately, making it concise and consistent with the existing code style.

submit_link_element = root.find('.//a[.="Submit"]')
submit_link = submit_link_element.get("href", "N/A") if submit_link_element is not None else "N/A"

Expand All @@ -330,6 +345,7 @@ def parse_task(html: AnyStr) -> Task:
description=description.strip(),
code=code,
submit_file=submit_file,
submit_link=submit_link,
)

return task
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 change looks concise, adding a submit_link parameter. Ensure that:

  1. submit_link is sanitized & validated (if it's user input).
  2. Check if any default value or null handling is needed for submit_link.
  3. Update relevant function documentation or comments to include submit_link.

Example for validation (assuming submit_link is a URL):

from urllib.parse import urlparse

def is_valid_url(url):
    try:
        result = urlparse(url)
        return all([result.scheme, result.netloc])
    except ValueError:
        return False

if not is_valid_url(submit_link):
    raise ValueError("Invalid submit link")

Expand All @@ -341,10 +357,13 @@ def print_task(task: Task) -> None:
print(f"\nSubmission file name: {task.submit_file}")


def submit_task(task_id: str, filename: str) -> None:
"""submit file to the submit form or task_id"""
# NOTE: use parse_form
...
# def submit_task(task_id: str, filename: str) -> None:
# """submit file to the submit form or task_id"""
# html = session.http_request(urljoin(base_url, f"task/{task_id}"))
# task = parse_task(html)
# answer = input("Do you want to submit this task? (y/n): ")
# if answer in ('y', 'Y'):
# with open(filename, 'r') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

The submit_task function is commented out, and its logic is moved to the main function. Consider refactoring the submission logic into a separate function for better maintainability.



def main() -> None:
Copy link

Choose a reason for hiding this comment

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

CODE REVIEW

  • Comments on Code Removal: Commenting out submit_task without removing it or replacing it implies uncertainty about its necessity. If the function is obsolete, delete it. If its functionality is being replaced or updated, ensure the new or updated code is included and clearly documented.

    # Consider removing or properly replacing this with updated functionality.

  • Incomplete Replacement Implementation: The snippet shows the start of a replacement for submit_task, but it's incomplete and inactive due to being commented. If this is a work in progress, finish the implementation, including error handling, actual submission logic, and feedback to the user on success or failure.

    def submit_task(task_id: str, filename: str) -> None:
    """Submit file to the task."""
    # Implementation goes here...
    # Ensure error handling, user feedback, and complete functionality.

  • Code Organization: Keep your code clean and organized. If submit_task is under modification, ensure it reflects the complete and final intent. Temporary or WIP code should be clearly marked or, preferably, kept out of the main branch until complete.

    # TODO: Complete this function before merging.

  • Functionality and User Input: If replacing submit_task, ensure you're including all necessary parts such as making the actual HTTP request to submit the task, handling potential errors, and validating the user's input more robustly.

    answer = input("Do you want to submit this task? (y/n): ").strip().lower()
if answer == 'y':
    # Proceed with submission.
else:
    # Handle 'no' response appropriately.

Remember, keeping your codebase clean and only merging complete, fully vetted features helps maintain overall code quality and reduces technical debt.

Expand Down Expand Up @@ -399,6 +418,51 @@ def main() -> None:
task = parse_task(html)
print_task(task)

if args.cmd == "submit":
html = session.http_request(urljoin(base_url, f"task/{args.task_id}"))
task = parse_task(html)
if not task.submit_file or task.submit_file == "N/A":
raise ValueError("No submission filename found")
if not task.submit_link or task.submit_link == "N/A":
raise ValueError("No submission link found")

submit_form_html = session.http_request(urljoin(base_url, task.submit_link))
submit_form_data = parse_form(submit_form_html)
action = submit_form_data.pop("_action")

for key, value in submit_form_data.items():
submit_form_data[key] = (None, value)
submit_form_data["file"] = (task.submit_file, open(task.submit_file, "rb"))
submit_form_data["lang"] = (None, "Python3")
submit_form_data["option"] = (None, "CPython3")

res = session.http_session.post(
urljoin(base_url, action), files=submit_form_data
)
html = res.text
result_url = res.url
print("Waiting for test results.", end="")
while "Test report" not in html:
print(".", end="")
sleep(1)
html = session.http_request(result_url)

print()
root = htmlement.fromstring(html)
submit_status_element = root.find('.//td[.="Status:"]/..') or Element("td")
submit_status_span_element = submit_status_element.find("td/span") or Element(
"span"
)
submit_status = submit_status_span_element.text or ""
submit_result_element = root.find('.//td[.="Result:"]/..') or Element("td")
submit_result_span_element = submit_result_element.find("td/span") or Element(
"span"
)
submit_result = submit_result_span_element.text or ""

print(f"Submission status: {submit_status.lower()}")
print(f"Submission result: {submit_result.lower()}")

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider refactoring the submission logic into a separate function for better readability and maintainability.

def submit_task(session, base_url, task_id, filename):
    html = session.http_request(urljoin(base_url, f"task/{task_id}"))
    task = parse_task(html)
    if not task.submit_file or task.submit_file == "N/A":
        raise ValueError("No submission filename found")
    if not task.submit_link or task.submit_link == "N/A":
        raise ValueError("No submission link found")

    submit_form_html = session.http_request(urljoin(base_url, task.submit_link))
    submit_form_data = parse_form(submit_form_html)
    action = submit_form_data.pop("_action")

    for key, value in submit_form_data.items():
        submit_form_data[key] = (None, value)
    submit_form_data["file"] = (task.submit_file, open(task.submit_file, "rb"))
    submit_form_data["lang"] = (None, "Python3")
    submit_form_data["option"] = (None, "CPython3")

    res = session.http_session.post(
        urljoin(base_url, action), files=submit_form_data
    )
    html = res.text
    result_url = res.url
    print("Waiting for test results.", end="")
    while "Test report" not in html:
        print(".", end="")
        sleep(1)
        html = session.http_request(result_url)

    print()
    root = htmlement.fromstring(html)
    submit_status_element = root.find('.//td[.="Status:"]/..') or Element("td")
    submit_status_span_element = submit_status_element.find("td/span") or Element("span")
    submit_status = submit_status_span_element.text or ""
    submit_result_element = root.find('.//td[.="Result:"]/..') or Element("td")
    submit_result_span_element = submit_result_element.find("td/span") or Element("span")
    submit_result = submit_result_span_element.text or ""

    print(f"Submission status: {submit_status.lower()}")
    print(f"Submission result: {submit_result.lower()}")

# In main function
if args.cmd == "submit":
    submit_task(session, base_url, args.task_id, args.filename)


if __name__ == "__main__":
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

Consider abstracting the new functionality into separate functions to improve readability and maintainability. Additionally, verifying file existence before attempting to open it improves robustness. Use with statement for file handling to ensure proper closure of the file. Refrain from directly printing dots in a loop as it's not efficient for user feedback in all environments; consider logging or other progress indication methods.

Example adjustments:

-        submit_form_data["file"] = (task.submit_file, open(task.submit_file, "rb"))
+        file_path = task.submit_file
+        if not os.path.exists(file_path):
+            raise FileNotFoundError(f"{file_path} does not exist")
+        with open(file_path, "rb") as file:
+            submit_form_data["file"] = (task.submit_file, file)
+            # Rest of file handling

Split large blocks into functions:

+    if args.cmd == "submit":
+        submit_task(args, session)

Refactoring this way enhances code structure, making it easier to manage and understand.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@senior-dev-bot can you suggests some unit tests for this method?