Remove leading $ from JSON expressions #30763
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -91,7 +91,7 @@ protected override void Print(ExpressionPrinter expressionPrinter) | |||
| { | |||
| expressionPrinter.Append("JsonScalarExpression(column: "); | |||
| expressionPrinter.Visit(JsonColumn); | |||
| expressionPrinter.Append($", {string.Join("", Path.Select(e => e.ToString()))})"); | |||
| expressionPrinter.Append($", {string.Join(".", Path.Select(e => e.ToString()))})"); | |||
There was a problem hiding this comment.
nit: maybe add quotes around the path? now that I look at the printer output in the translation failed tests below, it looks weird for the empty path. (also looked weird before, but now even more so)
There was a problem hiding this comment.
Sure, can do. It will get a bit weird if there's an expression within the string, but currently it seems to just do [(...)] for that. In any case it's just ToString, we can always change it.
| @@ -318,11 +318,11 @@ protected override Expression VisitExtension(Expression extensionExpression) | |||
| /// </summary> | |||
| protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExpression) | |||
| { | |||
| if (jsonScalarExpression.Path.Count == 1 | |||
| && jsonScalarExpression.Path[0].ToString() == "$") | |||
| // TODO: Look into not having empty JsonScalarExpression | |||
There was a problem hiding this comment.
separate into a different issue for tracking purposes?
And handle it in query SQL generation. Closes dotnet#30747
|
Hello @roji! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
1 similar comment
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
ghost
merged commit 
And handle it in query SQL generation.
Closes #30747