Make handling of ~ consistent, along with error handling.
#741
Conversation
|
@jph00 could you have a look? |
jph00
left a comment
jph00
left a comment
There was a problem hiding this comment.
Looks great. Just some minor suggestions.
| lines[dest_line-1:dest_line-1] = chunk | ||
| p.write_text('\n'.join(lines) + '\n') | ||
| return f"Moved lines {start_line}-{end_line} to line {dest_line}" | ||
| except Exception as e: return f'Error: {str(e)}' |
There was a problem hiding this comment.
The assertion errors have sufficient info in their descriptions, but other errors perhaps need to type info too?
| except Exception as e: return f'Error: {str(e)}' | |
| except AssertionError as e: return f'Error: {str(e)}' | |
| except Exception as e: return f'Error: {repr(e)}' |
| content[start_line-1:end_line] = [new_content] | ||
| p.write_text(''.join(content)) | ||
| return f"Replaced lines {start_line} to {end_line}." | ||
| except Exception as e: return f'Error replacing lines: {str(e)}' |
There was a problem hiding this comment.
This won't match the pattern "Error: " any more, which will break API clients relying on it. (Which I assume would be most clients - IIRC that's the pattern we use for indicating an error.)
| except Exception as e: return f'Error replacing lines: {str(e)}' | |
| except Exception as e: return f'Error: {repr(e)}' |
There was a problem hiding this comment.
Which downstream API are you referring to? Are we expecting these tools to be composed or used in places other than the LLM? I’d love to understand more about how you see the error strings being accessed?
|
I'm gonna make those minor changes at my end, since I need this PR for what I'm doing now. :D |
|
@jph00 Thank you for the megre and comments. I'm going to find a better way of getting notified about GitHub. I've noticed you haven't applied the changes yet so here is a new PR#745 that applies your suggestion partially. |
2 participants
While using solveit, I noticed
create("~/memo.md")was creating a file literally named~rather than expanding to the home directory.viewalready hadexpanduser().resolve(), but the other tools (create,insert,str_replace,replace_lines,move_lines) didn't.Rather than adding
expanduser().resolve()to each function individually, I've added a helper and wrappedreplace_lines/move_linesin try/except for consistent error handling. Also switched fromif not ... returntoassert.Let me know what you think.