support new file format from leetgo#5
Conversation
Summary of ChangesHello @zodman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
| try: | ||
| return _plain_text_input(content) | ||
| except Exception: | ||
| import traceback |
There was a problem hiding this comment.
Code Review
This pull request adds support for the leetgo test case format. The implementation introduces a new parsing function _plain_text_input and integrates it into load_test_cases.
My review found a couple of issues:
- The new parsing function
_plain_text_inputhas several bugs, including shadowing built-ins, and is not robust against malformed input, which could lead toAttributeErrororIndexError. - The new control flow in
load_test_casesfor trying different plain text formats contains a critical logic error with unreachable code and poor exception handling, which prevents the fallback parsing mechanism from working.
I've provided suggestions to fix these issues to make the new functionality more robust and correct. The other changes are minor stylistic improvements which look good.
Regarding your question in the PR description, yes, adding documentation for the newly supported format would be very helpful for users. You could update the docstring of load_test_cases or a relevant markdown file.
| def _plain_text_input(content): | ||
| """from leetgo format: https://github.com/j178/leetgo?tab=readme-ov-file#testcasestxt""" | ||
| raw_cases = content.split("\n\n") | ||
| _cases = [] | ||
| for raw_case in raw_cases: | ||
| lines = raw_case.splitlines() | ||
| input = [] | ||
| output = [] | ||
| res = None | ||
| for ln in lines: | ||
| if "input:" in ln: | ||
| res = input | ||
| continue | ||
| if "output:" in ln: | ||
| res = output | ||
| continue | ||
| res.append(json.loads(ln)) | ||
| _cases.append([input, output]) | ||
|
|
||
| return [[input, output[0]] for [input, output] in _cases] |
There was a problem hiding this comment.
This function for parsing the leetgo format has a few issues that make it fragile:
- It uses
inputandoutputas variable names, which shadow Python's built-in functions. This is generally discouraged. - If a test case chunk has content before the
input:marker,res.append()will be called onNone, raising anAttributeError. - The expression
output[0]will raise anIndexErrorif a test case is missing anoutput:section. - The check
if "input:" in lnis not robust and could match lines that are not the intended markers (e.g., a comment containing 'input:').
Consider refactoring this function to be more robust and readable. Here is a suggested implementation that addresses these points.
def _plain_text_input(content):
"""from leetgo format: https://github.com/j178/leetgo?tab=readme-ov-file#testcasestxt"""
raw_cases = content.strip().split("\n\n")
cases = []
for raw_case in filter(None, raw_cases):
lines = raw_case.splitlines()
input_data = []
output_data = []
current_section = None
for line in lines:
stripped = line.strip()
if stripped == "input:":
current_section = input_data
elif stripped == "output:":
current_section = output_data
elif current_section is not None and stripped:
current_section.append(json.loads(line))
if not output_data:
raise ValueError(f"Test case is missing 'output:' section or value: {raw_case}")
cases.append([input_data, output_data[0]])
return casesCo-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
ergs0204
left a comment
There was a problem hiding this comment.
Thanks for contributing this. Leetgo compatibility would be useful. Before merging, please add documentation plus tests covering multiple inputs, strings, blank output, malformed blocks, and fallback to Pyleet plain-text cases. The parser should also validate that both input: and output: are present: the current and condition accepts a block missing either marker, and the broad except Exception can hide malformed Leetgo data by silently falling back to the other parser. Once those cases are covered, I can review the behavior with confidence.
Your lib looks interesting.
Im just make some changes for my personal use. Should I improve it ?
I can make interoperation with leetgo, supporting his format of test cases... What do you think? should I add documentation ?