Support escaped slash in strings#246
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for escaped slash (\/) in YAML double-quoted strings to comply with YAML 1.2.0 specification, which requires this for strict JSON compatibility. This fixes issue #245 by porting the fix from the upstream go-yaml/yaml repository.
Changes:
- Added case handling for the
/escape character in the scanner's escape sequence parser - Added test case verifying that
\/in double-quoted strings decodes to/
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/libyaml/scanner.go | Added case handling for escaped slash (\/) in the flow scalar scanner, appending / to the result when encountered in double-quoted strings |
| decode_test.go | Added test case for issue #245 verifying that \/ in double-quoted strings correctly decodes to / and plain / remains unchanged |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ingydotnet
left a comment
ingydotnet
left a comment
There was a problem hiding this comment.
#240 is merged. It changed a lot, so you need to resolve conflicts.
Your commit msg isn't formatted to the repo rules.
Amend to remove the fix: part and it will be fine.
Tests are mostly defined as data now, so move the test as commented.
Thanks for the PR. It's a good one.
|
@hairyhenderson Do you have time or plans to make the required updates here? |
|
@ingydotnet apologies - been swamped. I'll have a look soon. |
hairyhenderson
force-pushed
the
support-escaped-slash-245
branch
from
8d6abac to
fdf28c7
Compare
hairyhenderson
changed the title
hairyhenderson
force-pushed
the
support-escaped-slash-245
branch
from
fdf28c7 to
2370356
Compare
|
OK I think I've addressed the comments - I ended up squashing and force-pushing as I was rebasing & amending the commit anyway... |
|
Interesting. You are fixing 2 known issues from yaml test suite. Could you adapt the known test failure file to remove the one that fails ? You can test locally with |
| // Support escaped slash ("\/"), as required by YAML 1.2.0+ | ||
| // for strict JSON compatibility | ||
| s = append(s, '/') |
There was a problem hiding this comment.
I can see this code from an old PR previously opened on a go-yaml/yaml
Here I'm unsure if the parsing of this shouldn't be triggered by an option.
I mean without this it's not accepted.
So now, it is. But the escape if this character is only accepted with YAML spec 1.2
It should remain invalid if we consider the yaml spec 1.1
Or should we consider the lib should by default accept it, and consider adding pure YAML spec 1.1 and YAML spec 1.2 options.
What do you think here @ingydotnet ?
|
|
||
| # https://github.com/yaml/go-yaml/issues/245 | ||
| - scalar-resolution: | ||
| name: Escaped slash in double-quoted string | ||
| yaml: '"\/"' | ||
| want: '/' |
There was a problem hiding this comment.
Shouldn't we add more tests? This sounds like an optimistic test that only tests what is expected ?
What other variations could we expect?
I'm unsure what to add here, but I raise the point just in case
| code_length = 4 | ||
| case 'U': | ||
| code_length = 8 | ||
| case '/': |
There was a problem hiding this comment.
I found back this comment from @ingydotnet in the old issue
Could we check if all expected characters is supported ?
| - scalar-resolution: | ||
| name: Escaped slash in double-quoted string | ||
| yaml: '"\/"' | ||
| want: '/' |
There was a problem hiding this comment.
Here a test is only added to the constructor. Don't we have other test files to update?
I mean the constructor is a low level one. I would expect to see code testing it with Load via the testdata suite in testdata/decode.yaml file
|
Sorry for the slow response. I have very little spare time these days. I'd appreciate any help to get this merged. Currently I'm using my fork and it's just fine for my needs, so my motivation to spend time on this is basically non-existent... |
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
ingydotnet
force-pushed
the
support-escaped-slash-245
branch
from
2370356 to
26ac207
Compare
Fixes #245 by porting forward go-yaml/yaml#862
Original text:
The JSON spec lists the
/character as optionally-escapable, and so YAML v1.2.0 added support for the sequence\/to be unescaped to/.This PR adds the minimum necessary support for this, and should solve the issues that a few people have already hit, as documented in go-yaml/yaml#797.
Thanks!