Check syntax of each solution #21
Conversation
|
Also, do you mind writing some unit tests? Currently the generate_solutions_pdf function has only a test for empty dir and another for a latex string which is correct. With you implementation, we could test for incorrect latex string examples. |
|
I have made some some changes based on your comments. Can you take a look again? I'll work on the unit tests. |
src/gpt_resolve/pdf_generator.py
Outdated
| if not validate_latex_content(content): | ||
| print(f"Error: Solution '{sol_file.name}' has LaTeX syntax errors. Please fix it.") | ||
| sol_files_with_issues.append(sol_file) | ||
| continue |
There was a problem hiding this comment.
Nice! The only thing left here is that any errors will count in sol_files_with_issues and loop will continue, but in the end the code will try to compile the whole pdf again, generating the same error that currently happens (for many questions, the traceback will be big).
So if you print errors only outside the loop in case of sol_files_with_issues not empty, it'd be solved:
for sol_file in solution_files:
...
if not validate_latex_content(content):
sol_files_with_issues.append(sol_file)
continue
if sol_files_with_issues:
raise ValueError("Errors in one or more solutions:\n' + '\n'.join(sol_files_with_issues) + '\n\nPlease check for syntax errors.")
...
|
@Krishn1412 do you have any updates here? |
|
Sorry about the delay @lgabs, I had some place to be. Can you take a look now? |
|
No problem @Krishn1412 , thanks a lot! Looks great now for merging. Do you have the time to add some unit test for invalid case? If so, I'll wait for your updates; if not, I'll create another issue to add this unit test afterwards. |
|
Sure @lgabs I'll add some unit tests. Do you want me to add them in the same PR or create a new one? |
|
You can do it in different PR, no problem. |
Solves #20