Copilot commented on code in PR #2425:
URL: https://github.com/apache/age/pull/2425#discussion_r3235534583
##########
drivers/python/age/builder.py:
##########
@@ -107,7 +107,12 @@ def visitAgValue(self, ctx:AgtypeParser.AgValueContext):
# Visit a parse tree produced by AgtypeParser#StringValue.
def visitStringValue(self, ctx:AgtypeParser.StringValueContext):
- return ctx.STRING().getText().strip('"')
+ # The STRING token always has surrounding '"' delimiters per the
+ # Agtype grammar. str.strip('"') would also drop any '"' characters
+ # that are part of the actual data when the value starts or ends
+ # with an escaped quote, e.g. '"foo \\"bar\\""' -> 'foo \\"bar\\',
+ # so trim exactly the first and last character instead.
+ return ctx.STRING().getText()[1:-1]
Review Comment:
The delimiter-trimming logic is now duplicated in both `visitStringValue()`
and `visitPair()`. Consider centralizing it in a small helper (e.g., a private
method) so any future changes to STRING token handling (or additional
validation/normalization) only need to be made in one place.
##########
drivers/python/age/builder.py:
##########
@@ -182,7 +187,8 @@ def visitPair(self, ctx:AgtypeParser.PairContext):
raise AGTypeError(ctx.getText(), "Missing key in object pair")
if agValNode is None:
raise AGTypeError(ctx.getText(), "Missing value in object pair")
- return (strNode.getText().strip('"') , agValNode)
+ # See visitStringValue() for why we slice instead of using strip('"').
+ return (strNode.getText()[1:-1] , agValNode)
Review Comment:
The delimiter-trimming logic is now duplicated in both `visitStringValue()`
and `visitPair()`. Consider centralizing it in a small helper (e.g., a private
method) so any future changes to STRING token handling (or additional
validation/normalization) only need to be made in one place.
##########
drivers/python/test_agtypes.py:
##########
@@ -245,6 +245,20 @@ def test_array_of_mixed_types(self):
self.assertEqual(result[4], [1, 2, 3])
self.assertEqual(result[5], {"key": "val"})
+ def test_string_value_preserves_inner_quotes(self):
+ """Issue #2418: visitStringValue must remove only the outer quote
+ delimiters, not every '"' on either side, otherwise values that end
+ with an escaped quote (e.g. '"foo \\"bar\\""') lose data."""
Review Comment:
This multi-line docstring is a bit hard to read as written. Consider
reformatting to follow typical Python docstring conventions (short summary
line, blank line, then details; closing quotes on their own line) to improve
readability and consistency with other tests.
##########
drivers/python/age/builder.py:
##########
@@ -182,7 +187,8 @@ def visitPair(self, ctx:AgtypeParser.PairContext):
raise AGTypeError(ctx.getText(), "Missing key in object pair")
if agValNode is None:
raise AGTypeError(ctx.getText(), "Missing value in object pair")
- return (strNode.getText().strip('"') , agValNode)
+ # See visitStringValue() for why we slice instead of using strip('"').
+ return (strNode.getText()[1:-1] , agValNode)
Review Comment:
There is an extra space before the comma in the returned tuple. Removing it
aligns with standard Python formatting conventions and avoids whitespace-only
churn in future diffs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
