Skip to content

Added support of collection initializer syntax.#250

Merged
davideicardi merged 7 commits intodynamicexpresso:masterfrom
holdenmai:fix_194_CollectionInit
Sep 26, 2022
Merged

Added support of collection initializer syntax.#250
davideicardi merged 7 commits intodynamicexpresso:masterfrom
holdenmai:fix_194_CollectionInit

Conversation

@holdenmai
Copy link
Copy Markdown
Contributor

Implementation of collection initializer syntax support for constructor #194 Managed to somehow close the original PR on accident during rebase operation pull 246, but all comments have now been resolved.

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]);
}
or 
 target.Eval<System.Collections.Generic.List<string>>("new List<string>(){string.Empty}");

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\"}}

Reference to method parameters is also supported like below

target.Eval<System.Collections.Generic.List<string>>("new List<string>(){StrProp = string.Empty}", new Parameter("StrProp", "0")) target.Eval<System.Collections.Generic.List<string>>("new List<string>(){StrValue()}", new Parameter("StrValue", new Func<string>(() => "Func")))

Copy link
Copy Markdown
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.

That looks really good! Apart from a few nitpicks, I'd recommend adding more tests, mostly to test the various possible exceptions. Also, I believe the current implementation can't parse new List<string> { { string.Empty } }.

Comment thread src/DynamicExpresso.Core/Parsing/Parser.cs Outdated
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs Outdated
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs Outdated
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs Outdated
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs Outdated
Comment thread src/DynamicExpresso.Core/Parsing/Parser.cs Outdated
Comment thread src/DynamicExpresso.Core/Parsing/Parser.cs
Comment thread src/DynamicExpresso.Core/Parsing/Parser.cs
Comment thread src/DynamicExpresso.Core/Parsing/Parser.cs Outdated
Comment thread src/DynamicExpresso.Core/Parsing/Parser.cs Outdated
@holdenmai holdenmai force-pushed the fix_194_CollectionInit branch from 0ac0258 to 990d579 Compare August 24, 2022 19:25
@holdenmai
Copy link
Copy Markdown
Contributor Author

I have added scenarios within the differing unit tests to cover each of the cases mentioned. Is it preferred that they become standalone cases to better show what negative case they are trying to cover?

@davideicardi
Copy link
Copy Markdown
Member

davideicardi commented Sep 17, 2022

Thank you @holdenmai

Is it preferred that they become standalone cases to better show what negative case they are trying to cover?

Yes, maybe having separate tests would be better... or try to simplify/explain better the existing ones.

…lustrate what compilation scenario they are covering.
@holdenmai
Copy link
Copy Markdown
Contributor Author

I have reorganized the unit tests to better show the different scenarios being covered.

@davideicardi
Copy link
Copy Markdown
Member

LGTM!

@metoule ?

There is just 3 conversations still open that I'm not sure if they are now resolved ...

Comment thread src/DynamicExpresso.Core/Parsing/Parser.cs
Comment thread src/DynamicExpresso.Core/Parsing/Parser.cs
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs Outdated
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs Outdated
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs
Comment thread test/DynamicExpresso.UnitTest/ConstructorTest.cs Outdated
Comment thread test/DynamicExpresso.UnitTest/ParametersTest.cs Outdated
@metoule
Copy link
Copy Markdown
Contributor

metoule commented Sep 21, 2022

Sorry for the late review! The code looks good to me, the test cases as well, I still have a few questions, but nothing major.

Copy link
Copy Markdown
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.

LGTM! Thank you!! 🚀

@davideicardi davideicardi merged commit 2b0661f into dynamicexpresso:master Sep 26, 2022
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