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

Support for dynamic types like expando object #36 #37

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Support for dynamic types like expando object #36 #37

wants to merge 5 commits into from

Conversation

PeterHagen
Copy link

Added validation if object inherits (or is) an IDictionary<string, object>, and return the value if the key exists

@VitaliyMF
Copy link
Contributor

I cannot accept this PR for these reasons:

  • a lot of changes doesn't change anything except spaces/formatting: https://github.com/nreco/lambdaparser/pull/37/files -- PR should contain only changes that are required to add a support for expando object.
  • all changes into csproj should be reverted, and target should remain netstandard20 which is compatible with all .NET platforms that are still supported (.NET Framework, .NET Core 3.1, NET5). Library should not target net50 unless this is necessary for something that exists only in net50 (but expando objects are not net50 specific), and even in this case existing targets should remain in csproj for nuget backward compatibility (+ net50 specific code should be wrapped with conditional compilation #IFs).
  • unit test(s) should be included for the proposed changes

As I understand only change that is actually needed is

			// An epandoObject has a IDictionary underneath
			object objectValue;
			if (obj is IDictionary<string, object> dictionary && dictionary.TryGetValue(propertyName, out objectValue))
			{
				return new LambdaParameterWrapper(objectValue, Cmp);
			}

To avoid possible misuse of other IDictionary<string, object> objects I think it is better to include a test for System.Dynamic.ExpandoObject type (obj is ExpandoObject).

You may change your PR to include only this change (+ add a unit test for it) or I may add support for expando object by myself.

@PeterHagen
Copy link
Author

Ah, sorry, I thought the pull request would only contain the first commit 09ffd3b, not the others. So yes, the change you quoted is the only change I suggest. I had to add the other changes to get it to run correctly for my situation, but just temporary.

To avoid possible misuse of other IDictionary<string, object> objects I think it is better to include a test for System.Dynamic.ExpandoObject type (obj is ExpandoObject).

I'm not sure what would happen with a dynamic object, and if it that would work with IDictionary too. So as ExpandoObject would be better.

After using this change I actually ran into another (logical) problem. Expando properties can have "-" symbols in their name. This won't work in LambdaParser, as it will (and I'm guessing) resolve in minus, instead of a string value. For example, I added a property "ODO-BMI-T-MC", and an expression like "diploma.ODO-BMI-T-MC == true" (where diploma is the expand object).

@VitaliyMF
Copy link
Contributor

After using this change I actually ran into another (logical) problem. Expando properties can have "-" symbols in their name. This won't work in LambdaParser, as it will (and I'm guessing) resolve in minus, instead of a string value. For example, I added a property "ODO-BMI-T-MC", and an expression like "diploma.ODO-BMI-T-MC == true" (where diploma is the expand object).

Since ExpandoObject implements IDictionary<string,object> you can access its properties simply like any other (real) dictionary?
Like that: diploma["ODO-BMI-T-MC"] == true

If this works it seems no need to add special support for ExpandoObject at all.

@PeterHagen
Copy link
Author

True, therefor I added a "safe" dictionary, which won't give exceptions on missing keys. So indeed, expandoObject won't be necessary at this moment, although I prefer expressions without [ ], to keep it clean for the users.

I'll remove my fork and please drop the request.

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.

None yet

2 participants