-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Handle % string escapes #256
Conversation
@richardmarshall Thanks, but recently I haven't had enough time to review it. will do it later this weekend 🙏 |
c96e740
to
3c3a62f
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need go fmt
probably
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.
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 |
3c3a62f
to
2a8c44a
Compare
Done. 👍 |
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%uXXXX
and 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