-
Notifications
You must be signed in to change notification settings - Fork 350
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
Fixes #2754, enable key in parenthesis after composable function with parameters #2757
base: release-7.x
Are you sure you want to change the base?
Conversation
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
|
||
Stack<ExpressionTokenKind> stack = new Stack<ExpressionTokenKind>(); | ||
stack.Push(ExpressionTokenKind.OpenParen); | ||
ExpressionLexer lexer = new ExpressionLexer(parenthesisExpression, true /*moveToFirstToken*/, false /*useSemicolonDelimiter*/, true /*parsingFunctionParameters*/); |
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.
Named parameters would be neater...
Stack<ExpressionTokenKind> stack = new Stack<ExpressionTokenKind>(); | ||
stack.Push(ExpressionTokenKind.OpenParen); | ||
ExpressionLexer lexer = new ExpressionLexer(parenthesisExpression, true /*moveToFirstToken*/, false /*useSemicolonDelimiter*/, true /*parsingFunctionParameters*/); | ||
bool paramertersFound = false; |
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.
nit:
bool paramertersFound = false; | |
bool parametersFound = false; |
@@ -1072,7 +1077,14 @@ private bool TryCreateSegmentForOperationImport(string identifier, string parent | |||
|
|||
this.parsedSegments.Add(segment); | |||
|
|||
this.TryBindKeySegmentIfNoResolvedParametersAndParenthesisValueExists(parenthesisExpression, returnType, resolvedParameters, segment); | |||
// Be noted, it's back-compatibile since the function can be called without parameters but with keys | |||
// for example: "~/GetCoolPeople(1)", where '1' is the key, not the parameters. |
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.
We know that "1" is a key and not a parameter because it's not named, no?
[InlineData("degree=1)('fawn'", "degree=1", "'fawn'")] | ||
public void SplitOperationParametersAndParenthesisKey_WorksForInputExpression(string expression, string parameters, string keys) | ||
{ | ||
FunctionParameterParser.SplitOperationParametersAndParenthesisKey(expression, out string acutalParams, out string actualKeys); |
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.
nit:
FunctionParameterParser.SplitOperationParametersAndParenthesisKey(expression, out string acutalParams, out string actualKeys); | |
FunctionParameterParser.SplitOperationParametersAndParenthesisKey(expression, out string actualParams, out string actualKeys); |
/// <summary> | ||
/// Splits the parenthesis expression into two parts (if apply) | ||
/// One is the function parameter, the other is key in parenthesis (if exists) | ||
/// Be noted, the input expression doesn't contain the beginning "(" and the ending ")" |
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.
Maybe this should be a <remark>
...
@@ -21,6 +21,19 @@ namespace Microsoft.OData.Tests.UriParser.Parsers | |||
/// </summary> | |||
public class FunctionParameterParserTests | |||
{ | |||
[Theory] | |||
[InlineData("1", "1", null)] |
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.
Is this a scenario where the function is being called with one parameter without specifying the name?
/// <param name="parenthesisKey">the output for key in parenthesis part</param> | ||
internal static void SplitOperationParametersAndParenthesisKey(string parenthesisExpression, out string parameters, out string parenthesisKey) | ||
{ | ||
// Be noted, the input expression doesn't contain the first '(' and last ')'. |
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.
Should we have a sanity check to confirm that the input expression does not containe the beginning "(" and the ending ")"?
[InlineData(")(1", "", "1")] | ||
[InlineData("person=People(1)", "person=People(1)", null)] | ||
[InlineData("person=People(1))('bca(aa('", "person=People(1)", "'bca(aa('")] | ||
[InlineData("degree=1)('fawn'", "degree=1", "'fawn'")] |
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.
Can you add the ""
scenario? No parameters and no keys, i.e. fn()
Assert.Equal(parameters, acutalParams); | ||
Assert.Equal(keys, actualKeys); | ||
} | ||
|
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.
Add a test for the scenario where the ODataException(ODataErrorStrings.ExpressionLexer_SyntaxError(currentToken.Position, parenthesisExpression));
is thrown
@@ -1072,7 +1077,14 @@ private bool TryCreateSegmentForOperationImport(string identifier, string parent | |||
|
|||
this.parsedSegments.Add(segment); | |||
|
|||
this.TryBindKeySegmentIfNoResolvedParametersAndParenthesisValueExists(parenthesisExpression, returnType, resolvedParameters, segment); | |||
// Be noted, it's back-compatibile since the function can be called without parameters but with keys | |||
// for example: "~/GetCoolPeople(1)", where '1' is the key, not the parameters. |
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.
How do we know (1)
represents the key and not the parameters?
… parameters
Issues
This pull request fixes #2754.
Description
Briefly describe the changes of this pull request.
Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.