-
Notifications
You must be signed in to change notification settings - Fork 16.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make json output parser handle newlines inside markdown code blocks #8682
Make json output parser handle newlines inside markdown code blocks #8682
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
fd46853
to
bb766f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the contribution! Code looks good, but expanding the documentation would be very helpful for other developers to better explain the intent of the code
return match.group(1) + value + match.group(3) | ||
|
||
|
||
def custom_parser(multiline_string: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind marking these as private functions?
Could you add a bit more documentation to explain what it means to "handle new lines and other special characters" correctly (i.e., explaining that special characters need to be escape properly, so this is searching for instances of special characters that aren't being escaped inside the action input
etc...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a glance at the code it's hard to tell whether the parser is exhaustive or whether there are more scenarios that will not be handled properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eyurtsev sure - updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: exhaustivity - I'm not sure. There may be other scenarios but I'm not aware of them. The main thing I was running into was single escaped \n
and unescaped "
breaking JSON parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eyurtsev ping.
thanks @bborn! |
Update to #8528
Newlines and other special characters within markdown code blocks returned as
action_input
should be handled correctly (in particular, unescaped"
=>\"
and\n
=>\\n
) so they don't break JSON parsing.@baskaryan