fix: support spaces in directive argument values#665
fix: support spaces in directive argument values#665rarguelloF wants to merge 8 commits intomainfrom
Conversation
There was a problem hiding this comment.
Can you add a test that demonstrates what happens in cases where the quoted string is broken (i.e, never closed / no space after close & before next / ...)? Those seem to be missing and they're the edge cases I worry about :)
There was a problem hiding this comment.
Done! added a few more test cases, please let me know if the behavior makes sense, or if there's any other test missing 🙏
| { | ||
| name: "unclosed double quote value", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `"my-service`}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "unclosed single quote value", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:'my-service`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: "'my-service"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "missing space between quoted args", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service"resource.name:"GET /"`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `my-service"resource.name:"GET /`}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "quote starts but ends with different quote type", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service'`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `"my-service'`}, | ||
| }, | ||
| wantOk: true, | ||
| }, |
There was a problem hiding this comment.
I find those surprising -- I reckon it's almost guaranteed that the outcome is not what the user intended...
There was a problem hiding this comment.
which one concretely? what do you think it should be the preferred behavior?
There was a problem hiding this comment.
I'd expect a lot of the "improperly terminated" cases to result in an error or a warning being emitted.
There was a problem hiding this comment.
@rarguelloF Ensure that the value is a valid Go string literal using double quote, fail on anything else (unless it's an unquoted string without spaces).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #665 +/- ##
==========================================
+ Coverage 65.72% 69.89% +4.17%
==========================================
Files 113 116 +3
Lines 7926 6929 -997
==========================================
- Hits 5209 4843 -366
+ Misses 2192 1541 -651
- Partials 525 545 +20
🚀 New features to boost your workflow:
|
3 participants
Adds support for spaces in directive argument values, either using single or double quotes, as in:
This is useful since its pretty common to use spaces in span or resource names, and in span tag values in general.