54 refactor split out client #59
Conversation
WalkthroughThe recent updates enhance the Changes
Poem
Tip Early Access Features
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 (
|
codiumai-pr-agent-pro
bot
added
enhancement
PR Review 🔍
|
PR Code Suggestions ✨
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
tyora/session.py (1)
Line range hint
41-77: Theloginmethod is well-refactored to handle the login process using direct credentials and HTML form parsing. Consider adding error handling for potential network issues during the login steps.try: # existing login code except requests.exceptions.RequestException as e: logger.error(f"Network error during login: {e}") raise
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- pyproject.toml (1 hunks)
- tests/test_session.py (3 hunks)
- tests/test_tyora.py (1 hunks)
- tests/test_utils.py (1 hunks)
- tyora/client.py (1 hunks)
- tyora/session.py (5 hunks)
- tyora/tyora.py (1 hunks)
- tyora/utils.py (1 hunks)
Files skipped from review due to trivial changes (2)
- pyproject.toml
- tests/test_tyora.py
Additional comments not posted (15)
tests/test_session.py (3)
23-23: The test for successful login correctly uses the updatedloginmethod signature and properly mocks the HTTP responses.
44-44: The test for failed login is well-implemented, correctly anticipating aValueErrorwhen login credentials are incorrect.
44-44: The cookie handling test is straightforward and effectively checks the session's cookie management.tests/test_utils.py (2)
15-22: The tests inTestFindLinkcover both successful and unsuccessful scenarios effectively, ensuring thefind_linkfunction behaves as expected under various conditions.
36-46: The tests inTestParseFormare comprehensive, covering cases with and without forms and inputs, ensuring robust behavior of theparse_formfunction.tyora/session.py (2)
38-39: Theis_logged_inmethod correctly uses thefind_linkfunction to check for a logout link, which is a reliable indicator of session status.
22-22: The constructor ofMoocfiCsesSessionis correctly set up to initialize the session with the provided base URL and cookies.tyora/client.py (2)
85-127: Theparse_task_listfunction effectively parses HTML to construct a list ofTaskobjects, ensuring each task is correctly identified and categorized.
130-174: Theparse_taskfunction is thorough in extracting detailed information about tasks from HTML, ensuring all relevant data is captured and properly formatted.tyora/tyora.py (6)
32-89: Theparse_argsfunction is well-implemented, providing a comprehensive set of command-line options and handling different commands effectively.
92-100: Thecreate_configfunction correctly prompts for user credentials and constructs the configuration dictionary.
115-122: Theread_configfunction effectively reads the configuration from a file and checks for the presence of essential settings.
125-140: Theread_cookie_filefunction is robust, handling potential errors such as file not found and JSON decoding issues effectively.
161-171: Theprint_task_listfunction is well-implemented, correctly applying filters and limits to the list of tasks displayed.
173-177: Theprint_taskfunction effectively displays detailed information about a task, including its description and submission file name.
tyora/utils.py
Outdated
| def find_link(html: AnyStr, xpath: str) -> dict[str, Optional[str]]: | ||
| """Search for html link by xpath and return dict with href and text""" | ||
| anchor_element = html5lib.parse(html, namespaceHTMLElements=False).find(xpath) | ||
| if anchor_element is None: | ||
| return dict() | ||
|
|
||
| link_data = dict() | ||
| link_data["href"] = anchor_element.get("href") | ||
| link_data["text"] = anchor_element.text | ||
|
|
||
| return link_data |
There was a problem hiding this comment.
The implementation of find_link looks good. Consider adding unit tests to cover edge cases, such as malformed HTML or incorrect XPath expressions.
Would you like me to help with writing these tests?
tyora/utils.py
Outdated
| 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 = html5lib.parse(html, namespaceHTMLElements=False).find(xpath) | ||
| if form_element is None: | ||
| return dict() | ||
|
|
||
| form_data = dict() | ||
| form_data["_action"] = form_element.get("action") | ||
| for form_input in form_element.iter("input"): | ||
| form_key = form_input.get("name") or "" | ||
| form_value = form_input.get("value") or "" | ||
| form_data[form_key] = form_value | ||
|
|
||
| return form_data |
There was a problem hiding this comment.
The implementation of parse_form is robust and handles cases where the form or inputs are missing. Consider adding unit tests to ensure it handles various HTML structures and edge cases correctly.
Would you like assistance in creating these tests?
tyora/client.py
Outdated
| class Client: | ||
| def __init__(self, session: Session) -> None: | ||
| self.session = session | ||
|
|
||
| def login(self, username: str, password: str) -> None: | ||
| self.session.login(username, password) | ||
|
|
||
| def get_task_list(self) -> list[Task]: | ||
| res = self.session.get(urljoin(self.session.base_url, "list")) | ||
| res.raise_for_status() | ||
| return parse_task_list(res.text) | ||
|
|
||
| def get_task(self, task_id: str) -> Task: | ||
| res = self.session.get(urljoin(self.session.base_url, f"task/{task_id}")) | ||
| res.raise_for_status() | ||
| try: | ||
| task = parse_task(res.text) | ||
| except ValueError as e: | ||
| logger.debug(f"Error parsing task: {e}") | ||
| raise | ||
| return task | ||
|
|
||
| def submit_task( | ||
| self, task_id: str, submission: AnyStr, filename: Optional[str] | ||
| ) -> str: | ||
| task = self.get_task(task_id) | ||
| if not task.submit_file and not filename: | ||
| raise ValueError("No submission filename found") | ||
| if not task.submit_link: | ||
| raise ValueError("No submit link found") | ||
| submit_file = task.submit_file or filename | ||
|
|
||
| res = self.session.get(urljoin(self.session.base_url, task.submit_link)) | ||
| res.raise_for_status() | ||
| parsed_form_data = parse_form(res.text) | ||
| action = parsed_form_data.pop("_action") | ||
|
|
||
| submit_form_data = dict() | ||
| for key, value in parsed_form_data.items(): | ||
| submit_form_data[key] = (None, value) | ||
| submit_form_data["file"] = (submit_file, submission) | ||
| submit_form_data["lang"] = (None, "Python3") | ||
| submit_form_data["option"] = (None, "CPython3") | ||
| res = self.session.post( | ||
| urljoin(self.session.base_url, action), files=submit_form_data | ||
| ) | ||
| res.raise_for_status() | ||
|
|
||
| return res.url | ||
|
|
There was a problem hiding this comment.
The Client class is well-designed to handle server interactions for tasks. It uses the session effectively for HTTP requests. Consider adding error handling for network issues to improve robustness.
try:
# existing client code
except requests.exceptions.RequestException as e:
logger.error(f"Network error during client operations: {e}")
raise| def write_config(configfile: str, config: dict[str, str]) -> None: | ||
| file_path = Path(configfile).expanduser() | ||
| if file_path.exists(): | ||
| # TODO: https://github.com/madeddie/tyora/issues/28 | ||
| ... | ||
| # Ensure directory exists | ||
| file_path.parent.mkdir(parents=True, exist_ok=True) | ||
| print("Writing config to file") | ||
| with open(file_path, "w") as f: | ||
| json.dump(config, f) |
There was a problem hiding this comment.
The write_config function is well-implemented, ensuring the configuration directory exists before writing. Consider adding error handling for file write operations to improve robustness.
try:
with open(file_path, "w") as f:
json.dump(config, f)
except IOError as e:
logger.error(f"Failed to write config to {file_path}: {e}")
raise
tyora/tyora.py
Outdated
| def main() -> None: | ||
| args = parse_args() | ||
|
|
||
| logging.basicConfig( | ||
| level=logging.DEBUG if args.debug else logging.WARNING, | ||
| format="%(asctime)s %(levelname)s %(message)s", | ||
| datefmt="%Y-%m-%d %H:%M:%S", | ||
| ) | ||
|
|
||
| if args.cmd == "login": | ||
| config = create_config() | ||
| write_config(args.config, config) | ||
| return | ||
|
|
||
| config = read_config(args.config) | ||
|
|
||
| # 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']}/" | ||
|
|
||
| cookiefile = None | ||
| cookies = dict() | ||
| if not args.no_state: | ||
| if not STATE_DIR.exists(): | ||
| STATE_DIR.mkdir(parents=True, exist_ok=True) | ||
| cookiefile = STATE_DIR / "cookies.json" | ||
| cookies = read_cookie_file(str(cookiefile)) | ||
|
|
||
| session = Session( | ||
| base_url=base_url, | ||
| cookies=cookies, | ||
| ) | ||
|
|
||
| client = Client(session) | ||
| client.login(username=config["username"], password=config["password"]) | ||
|
|
||
| if not args.no_state and cookiefile: | ||
| cookies = session.cookies.get_dict() | ||
| write_cookie_file(str(cookiefile), cookies) | ||
|
|
||
| if args.cmd == "list": | ||
| print_task_list(client.get_task_list(), filter=args.filter, limit=args.limit) | ||
|
|
||
| if args.cmd == "show": | ||
| print_task(client.get_task(args.task_id)) | ||
|
|
||
| if args.cmd == "submit": | ||
| # TODO allow user to paste the code in or even pipe it in | ||
| with open(args.filename, "rb") as f: | ||
| submission_code = f.read() | ||
|
|
||
| result_url = client.submit_task( | ||
| task_id=args.task_id, | ||
| filename=args.filename, | ||
| submission=submission_code, | ||
| ) | ||
| print("Waiting for test results.", end="") | ||
| while True: | ||
| print(".", end="") | ||
| res = session.get(result_url) | ||
| print(res.text) | ||
| res.raise_for_status() | ||
| if "Test report" in res.text: | ||
| break | ||
| sleep(1) | ||
|
|
||
| print() | ||
| results = parse_submit_result(res.text) | ||
|
|
||
| print(f"Submission status: {results['status']}") | ||
| print(f"Submission result: {results['result']}") | ||
|
|
There was a problem hiding this comment.
The main function is well-structured, handling different commands based on user input effectively. Consider adding error handling for network issues to ensure the script is robust under various conditions.
try:
# existing main function code
except requests.exceptions.RequestException as e:
logger.error(f"Network error during operations: {e}")
raise
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
requirements-dev.lockis excluded by!**/*.lockrequirements.lockis excluded by!**/*.lock
Files selected for processing (7)
- pyproject.toml (1 hunks)
- tests/test_session.py (4 hunks)
- tests/test_tyora.py (1 hunks)
- tyora/client.py (1 hunks)
- tyora/session.py (5 hunks)
- tyora/tyora.py (1 hunks)
- tyora/utils.py (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- pyproject.toml
- tests/test_session.py
- tests/test_tyora.py
- tyora/tyora.py
- tyora/utils.py
Additional comments not posted (6)
tyora/session.py (2)
23-23: Constructor setup looks good and is well-documented.
40-41: Method to check login status is correctly implemented.tyora/client.py (4)
39-41: Login method correctly delegates to the session object.
42-45: Method to retrieve task list is implemented correctly.
47-55: Error handling in the task retrieval method is robust.
87-176: Utility functions for parsing tasks are well-implemented.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ora into 54-refactor-split-out-client
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- tests/test_client.py (1 hunks)
- tests/test_data/session_logged_in_some_tasks_done.html (1 hunks)
- tests/test_data/task_3052_incomplete_no_submit_link.html (1 hunks)
- tests/test_data/task_3055_complete.html (1 hunks)
- tyora/client.py (1 hunks)
- tyora/session.py (5 hunks)
- tyora/tyora.py (1 hunks)
Files skipped from review due to trivial changes (2)
- tests/test_data/task_3052_incomplete_no_submit_link.html
- tests/test_data/task_3055_complete.html
Files skipped from review as they are similar to previous changes (1)
- tyora/tyora.py
Additional comments not posted (9)
tests/test_data/session_logged_in_some_tasks_done.html (1)
40-40: The updated task list in the HTML content aligns with the changes described in the PR summary.tyora/session.py (2)
23-23: The updated constructor aligns with the PR's objective to simplify session management by reducing the parameters.
Line range hint
43-80: The refactoring of theloginmethod improves readability and maintainability. The handling of the_actionvariable is now more explicit, addressing previous concerns.tests/test_client.py (3)
24-45: The test case for retrieving the task list is well-implemented, with correct assertions that align with the expected task details.
48-65: The test case for retrieving a complete task is thorough, with detailed assertions that correctly validate the task's properties and submission link.
67-85: The test case for an incomplete task without a submit link is correctly implemented, with appropriate assertions for the task's properties.tyora/client.py (3)
39-42: Theget_task_listmethod is correctly implemented, using the session to retrieve and parse the task list.
44-52: Theget_taskmethod is well-implemented, with appropriate error handling and logging for task retrieval.
54-80: Thesubmit_taskmethod is robust, correctly handling the submission process, including checks for necessary submission details and handling form data.
There was a problem hiding this comment.
Actionable comments posted: 4
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- pyproject.toml (1 hunks)
- tests/test_client.py (1 hunks)
- tyora/client.py (1 hunks)
- tyora/tyora.py (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- tests/test_client.py
- tyora/tyora.py
Additional comments not posted (3)
tyora/client.py (3)
19-22: TheTaskStateenum is well-defined and clear.
24-33: TheTaskdataclass is well-structured and appropriately uses optional fields for properties that may not always be present.
59-61: The error messages have been improved to include the task ID, which enhances the clarity and debuggability of the code.
| class Client: | ||
| def __init__(self, session: Session) -> None: | ||
| self.session = session | ||
|
|
||
| def get_task_list(self) -> list[Task]: | ||
| res = self.session.get(urljoin(self.session.base_url, "list")) | ||
| res.raise_for_status() | ||
| return parse_task_list(res.text) | ||
|
|
||
| def get_task(self, task_id: str) -> Task: | ||
| res = self.session.get(urljoin(self.session.base_url, f"task/{task_id}")) | ||
| res.raise_for_status() | ||
| try: | ||
| task = parse_task(res.text) | ||
| except ValueError as e: | ||
| logger.debug(f"Error parsing task: {e}") | ||
| raise | ||
| return task | ||
|
|
||
| def submit_task( | ||
| self, task_id: str, submission: str, filename: Optional[str] | ||
| ) -> str: | ||
| task = self.get_task(task_id) | ||
| if not task.submit_file and not filename: | ||
| raise ValueError("No submission filename found for task ID: " + task_id) | ||
| if not task.submit_link: | ||
| raise ValueError("No submit link found for task ID: " + task_id) | ||
| submit_file = task.submit_file or filename | ||
|
|
||
| res = self.session.get(urljoin(self.session.base_url, task.submit_link)) | ||
| res.raise_for_status() | ||
| parsed_form_data = parse_form(res.text) | ||
| action = parsed_form_data.pop("_action") | ||
|
|
||
| submit_form_data: dict[str, tuple[Optional[str], Optional[str]]] = dict() | ||
| for key, value in parsed_form_data.items(): | ||
| submit_form_data[key] = (None, value) | ||
| submit_form_data["file"] = (submit_file, submission) | ||
| submit_form_data["lang"] = (None, "Python3") | ||
| submit_form_data["option"] = (None, "CPython3") | ||
| res = self.session.post( | ||
| urljoin(self.session.base_url, action), | ||
| files=submit_form_data, # type: ignore[arg-type] | ||
| ) | ||
| res.raise_for_status() | ||
| return res.url | ||
|
|
There was a problem hiding this comment.
The Client class methods are well-organized and handle server interactions effectively. However, consider adding error handling for network issues to improve robustness, as previously suggested.
Would you like me to add the error handling code for network issues as previously suggested?
| def parse_task_list(html: AnyStr) -> list[Task]: | ||
| """Parse html to find tasks and their status, returns list of Task objects""" | ||
| root = html5lib.parse(html, namespaceHTMLElements=False) # type: ignore[reportUnknownMemberType] | ||
| task_element_list = root.findall('.//li[@class="task"]') | ||
|
|
||
| task_list: list[Task] = list() | ||
| for task_element in task_element_list: | ||
| task_id = None | ||
| task_name = None | ||
| task_state = None | ||
|
|
||
| task_link = task_element.find("a") | ||
| if task_link is None: | ||
| continue | ||
|
|
||
| task_name = task_link.text | ||
| task_id = task_link.get("href", "/").split("/")[-1] | ||
| if not task_name or not task_id: | ||
| continue | ||
|
|
||
| task_element_spans = task_element.findall("span") | ||
| if not task_element_spans: | ||
| continue | ||
|
|
||
| task_element_span = next( | ||
| (span for span in task_element_spans if span.get("class", "") != "detail"), | ||
| Element("span"), | ||
| ) | ||
| task_element_class = task_element_span.get("class") or "" | ||
|
|
||
| task_state = ( | ||
| TaskState.COMPLETE if "full" in task_element_class else TaskState.INCOMPLETE | ||
| ) | ||
|
|
||
| task = Task( | ||
| id=task_id, | ||
| name=task_name, | ||
| state=task_state, | ||
| ) | ||
| task_list.append(task) | ||
|
|
||
| return task_list | ||
|
|
There was a problem hiding this comment.
The parse_task_list function is comprehensive and handles various edge cases well. However, consider adding error handling for unexpected HTML structures to prevent runtime errors.
Would you like me to add error handling for unexpected HTML structures in the parse_task_list function?
| def parse_task(html: AnyStr) -> Task: | ||
| root = html5lib.parse(html, namespaceHTMLElements=False) # type: ignore[reportUnknownMemberType] | ||
| task_link_element = root.find('.//div[@class="nav sidebar"]/a[@class="current"]') | ||
| task_link = task_link_element if task_link_element is not None else Element("a") | ||
| task_id = task_link.get("href", "").split("/")[-1] | ||
| if not task_id: | ||
| raise ValueError("Failed to find task id") | ||
| task_name = task_link.text or None | ||
| if not task_name: | ||
| raise ValueError("Failed to find task name") | ||
| 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", None) | ||
| submit_link_element = root.find('.//a[.="Submit"]') | ||
| submit_link = ( | ||
| submit_link_element.get("href", None) | ||
| if submit_link_element is not None | ||
| else None | ||
| ) | ||
|
|
||
| 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 | ||
| ] | ||
| ), | ||
| None, | ||
| ) | ||
| 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, | ||
| submit_link=submit_link, | ||
| ) | ||
|
|
||
| return task |
There was a problem hiding this comment.
The parse_task function is detailed and robust, extracting various task properties from HTML. However, the use of html5lib without proper error handling could lead to issues if the HTML structure changes.
Would you like me to add error handling for unexpected HTML structures in the parse_task function?
| # TODO test with failing results | ||
| # Seems to be broken since the switch to html5lib, needs tests! | ||
| def parse_submit_result(html: AnyStr) -> dict[str, str]: | ||
| root = html5lib.parse(html, namespaceHTMLElements=False) # type: ignore[reportUnknownMemberType] | ||
| 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 "" | ||
|
|
||
| return { | ||
| "status": submit_status.lower(), | ||
| "result": submit_result.lower(), | ||
| } |
There was a problem hiding this comment.
The parse_submit_result function appears to be a work in progress, as indicated by the TODO comment. It's crucial to add tests to ensure its functionality, especially since the switch to html5lib.
Would you like me to help with writing tests for the parse_submit_result function?
The flow is not completed in the test, to be revisited
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- tests/test_client.py (1 hunks)
- tests/test_data/submit_3055_form.html (1 hunks)
- tests/test_data/task_3052_incomplete_no_submit_link.html (1 hunks)
- tests/test_data/task_3055_complete.html (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/test_data/submit_3055_form.html
Files skipped from review as they are similar to previous changes (3)
- tests/test_client.py
- tests/test_data/task_3052_incomplete_no_submit_link.html
- tests/test_data/task_3055_complete.html
|
Merging this work now, adding a new issue to fix the parse_submit_results and its tests. |
Also removing the src layer since I find it annoying, might put it back at a later date.
Summary by CodeRabbit
New Features
Tests
Clientclass to ensure reliable task management and interaction with the server.