Skip to content
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

Added support of collection initializer syntax. #246

Closed

Conversation

holdenmai
Copy link
Contributor

@holdenmai holdenmai commented Aug 15, 2022

Implementation of collection initializer syntax support for constructor #194 Copied a little bit of conflicting code from pull 245.

You can now write

var target = new Interpreter();
target.Reference(typeof(System.Collections.Generic.List<>));
var intList = target.Eval<System.Collections.Generic.List<int>>("new List<int>(){1, 2, 3, 4, 5}");
Assert.AreEqual(5, intList.Count);
for (int i = 0; i < intList.Count; ++i)
{
    Assert.AreEqual(i + 1, intList[i]);
}

as well as implementations supporting multiple parameters in the Add method such as

new MyClassAdder(){{ 1, 2, 3, 4, 5},{\"6\" },7 }
and
new Dictionary<int, string>(){{1, \"1\"}, {2, \"2\"}, {3, \"3\"}, {4, \"4\"}, {5, \"5\"}}

Copy link
Contributor

@metoule metoule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good, thank you!! The only problem I can see is that you can't use any expression that starts with an identifier in the collection initializer list:

new List<string> { string.Empty }

is valid, but not accepted with your code.

var member = FindPropertyOrField(newType, propertyOrFieldName, false);
if (member == null)
throw CreateParseException(_token.pos, ErrorMessages.UnknownPropertyOrField, propertyOrFieldName, GetTypeName(newType));
if (_token.id != TokenId.Identifier)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you perform the check, but that's a bit limiting, because an identifier can be used:

new List<string> { string.Empty } 

You can peek at the next token to check if it's a equal sign: if it's not, you can use SetTextPos to restart parsing at the wanted location and call ParseExpressionSegment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having some trouble getting that to work when I was trying to account for the invalid case of
new MyClassAdder(){{StrProp = "SomeValue"}}
but I will try again according to your review comments

Copy link
Contributor Author

@holdenmai holdenmai Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in latest commit.

Also accounted for the ability to perform collection of the .Add while simultaneously setting a variable/parameter value even if that matches a property/field name.

var target = new Interpreter();
			target.Reference(typeof(MyClassAdder));
			Assert.DoesNotThrow(() => target.Eval<MyClassAdder>("new MyClassAdder(){{ 1, 2, 3, 4, 5},{StrProp = \"6\" },7}", new Parameter("StrProp", "0")));

This is the equivalent of

string StrProp = "";
var mc = new MyClassAdder()
{
{1,2,3 },
{StrProp = "6" }
};
       
Assert.AreEqual(StrProp, "6")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was so focused on the improvements around the checking of the additional cases, I missed the original List work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @holdenmai . Also remember to update/rebase your branch so that we will not have conflicts during merge.

src/DynamicExpresso.Core/Parsing/Parser.cs Outdated Show resolved Hide resolved
src/DynamicExpresso.Core/Parsing/Parser.cs Outdated Show resolved Hide resolved
src/DynamicExpresso.Core/Parsing/Parser.cs Outdated Show resolved Hide resolved
src/DynamicExpresso.Core/Parsing/Parser.cs Outdated Show resolved Hide resolved
holdenmai and others added 3 commits August 15, 2022 11:52
Misc style changes from PR review
…hen calling the collection initialization syntax.
@holdenmai holdenmai closed this Aug 20, 2022
@holdenmai holdenmai deleted the fix_194_CollectionInit branch August 20, 2022 19:08
@holdenmai holdenmai restored the fix_194_CollectionInit branch August 20, 2022 19:08
@holdenmai holdenmai deleted the fix_194_CollectionInit branch August 20, 2022 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants