Handle % string escapes #256
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@richardmarshall Thanks, but recently I haven't had enough time to review it. will do it later this weekend 🙏 |
This was referenced Feb 15, 2024
richardmarshall
force-pushed
the
string_escapes
branch
from
c96e740
to
3c3a62f
Compare
ysugimoto
approved these changes
Feb 18, 2024
There was a problem hiding this comment.
Awesome implementation, your approach makes us more robust for string dealing.
and, postfix expression is also nice, we can implement it easier if we need more prefix units.
I need to reflect this change on #248 but I will do 👍
I put a tiny nit but we can merge.
| @@ -90,8 +90,11 @@ func TestProcessDeclarations(t *testing.T) { | |||
| DirectorType: &ast.Ident{Value: "client"}, | |||
| Properties: []ast.Expression{ | |||
| &ast.DirectorProperty{ | |||
| Key: &ast.Ident{Value: "quorum"}, | |||
| Value: &ast.String{Value: "20%"}, | |||
| Key: &ast.Ident{Value: "quorum"}, | |||
There was a problem hiding this comment.
Since the Value literal is now on multiple lines the alignment of Key with it was actually removed by go fmt.
|
@richardmarshall Could you resolve conflicts that cause by another PRs? thanks |
richardmarshall
force-pushed
the
string_escapes
branch
from
3c3a62f
to
2a8c44a
Compare
Done. 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we are parsing string escapes using the more common
\style. VCL string escapes use a different format and are considerably more complicated.The three forms of escapes supported by VCL are UTF-8 byte escapes
%XX, fixed width unicode code point escapes%uXXXXand variable width unicode code point escapes%u{...}.The utf-8 escapes represent bytes of a encoded escape and can be between 1 and 4 sequential escapes. The rules for utf-8 encoding are enforced and the decoded code point must be a valid. Arbitrary byte sequences of binary data is not supported.
The two forms of unicode code point escapes are differentiated by the fact that the fixed width escapes can only represent code points up to 0xFFFF while the variable width escapes support up to 0x10FFFF.
For all escape types a zero value is treated as a terminator for the string and the remaining portion of the string is dropped.
More information here:
https://developer.fastly.com/reference/vcl/types/string/
This PR removes the
\handling logic and adds support for the 3%escape types.Additionally due to the fact that the quorum field for directors is represented as a percentage with a
%the handling of these percentage expressions is updated as well to not be treated as a string. This also fixes a compatibility issue due to the fact that Fastly treats these percentages as a postfix expression.Meaning code like this is valid:
I implemented this type of expression with a generic postfix expression handler for the
%operator. After finishing that I think maybe it's unnecessary to support the general case postfix expression as the quorum percentage is as far as I can tell the only use case in VCL. I've left the general case implementation in place for now but let me know if you think that simplifying this and only supporting the percentage expression is the right way to go.Resolves #246