[OMEGA-174] Multi-line command argument parsing of MeTTaClaw#223
[OMEGA-174] Multi-line command argument parsing of MeTTaClaw#223patham9 wants to merge 3 commits into
Conversation
|
The autotest with |
The reason test fails is additional quote inside the string: My opinion the behavior of your code @patham9 is correct in this sense. I would suggest fixing the test assert condition: - assert balance_parentheses('send "Plain text version:"\n**Mars** - red planet\nNote: Pluto is a dwarf planet') == '((send "Plain text version:\\n**Mars** - red planet\\nNote: Pluto is a dwarf planet"))'
+ assert balance_parentheses('send "Plain text version:"\n**Mars** - red planet\nNote: Pluto is a dwarf planet') == '((send "\\\"Plain text version:\\\"\\n**Mars** - red planet\\nNote: Pluto is a dwarf planet"))' |
vsbogd
left a comment
There was a problem hiding this comment.
I agree this change improves the previous implementation. I would also ask to add the unit tests which checks that multi-line parsing works for the commands other than send.
| def _strip_outer_parens(line): | ||
| if line.startswith("(") and line.endswith(")"): | ||
| return line[1:-1].strip() | ||
| return line | ||
|
|
There was a problem hiding this comment.
This function is not used and could be removed:
| def _strip_outer_parens(line): | |
| if line.startswith("(") and line.endswith(")"): | |
| return line[1:-1].strip() | |
| return line |
| if cur: | ||
| cur.append(raw) |
There was a problem hiding this comment.
Not sure why empty string is added into the current block? No tests are broken if it is removed:
| if cur: | |
| cur.append(raw) |
There was a problem hiding this comment.
If it executes commands like write-file, the additional newlines can matter. This just preserves this, as else it would truncate them.
| def balance_parentheses(s): | ||
| s = s.replace("_quote_", '"').replace("_newline_", "\n") | ||
| sexprs = [] | ||
| special_two_arg_cmds = {"write-file", "append-file"} |
There was a problem hiding this comment.
This deserves to be a global constant like LLM_COMMANDS.
There was a problem hiding this comment.
I agree. Maybe TWO_ARG_COMMANDS
| if line.startswith("(-"): | ||
| line = "(pin -" + line[2:] | ||
| elif line.startswith("-"): | ||
| line = "pin " + line |
There was a problem hiding this comment.
@patham9 why this replacement is needed? No test is broken if removed.
There was a problem hiding this comment.
Not needed anymore probably, was more an issue with Minimax M2.7 that it used listings without command.
| content = split_rest[1].strip() if len(split_rest) > 1 else "" | ||
| if content: | ||
| if content.startswith('"') and content.endswith('"'): | ||
| if content.startswith('"') and content.endswith('"') and "\n" not in content: |
There was a problem hiding this comment.
Could we change the code of quote_arg to include this condition. I.e. make quote_args adding quotes only if they are absent. If so it would significantly simplify the code by removing this condition from balance_parenthesis function. For example this condition will turn to:
sexprs.append(f"({cmd} {filename} {quote_arg(content)})")the same for if rest block below.
There was a problem hiding this comment.
I am not sure about this, but it should be fine too.
Description
Multi-line argument was restricted to
sendcommand, and parsing seemed more complex than necessary.How Has This Been Tested?
Tested with MeTTaClaw live Max Botnick instance.
Checklist