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

Add test code to validate boolean value in the if condition. #1803

Merged
merged 7 commits into from
Mar 14, 2024

Conversation

hyzx86
Copy link
Contributor

@hyzx86 hyzx86 commented Mar 13, 2024

relate #1801

@hyzx86 hyzx86 marked this pull request as draft March 13, 2024 06:52
@lahma
Copy link
Collaborator

lahma commented Mar 13, 2024

Why don't you just investigate the problem instead making others do the leg work?

@hyzx86
Copy link
Contributor Author

hyzx86 commented Mar 13, 2024

Why don't you just investigate the problem instead making others do the leg work?

Sorry, I've already made some attempts. But none of them worked as expected

@hyzx86 hyzx86 marked this pull request as ready for review March 13, 2024 09:17
@hyzx86
Copy link
Contributor Author

hyzx86 commented Mar 13, 2024

Hi @lahma , I tried to use AddObjectConverter to solve this problem, so far the test passes, but I don't know if the method is correct.

Could you help to review it? thank you.

namespace Jint.Tests.PublicInterface;

#if NET8_0_OR_GREATER
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is supposed to be here, is it? At least I don't see any .NET 8 specific code at a glance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

            var valueKind = jsonValue.GetValueKind();

This line ,the GetValueKind() is warning not support net462

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that method was introduced with .NET 8...but the available NuGet package that ships System.Text.Json still supports .NET Framework 4.6.2 upwards; so it should work there as well. If this wasn't the case, the old code should've errored out already too.

Granted, I don't have any STJ interop scenarios on .NET Framework at the moment, but I still use this in a .NET 6 integration that is waiting to get upgraded to .NET 8. And as far as I can tell, things should work there if you override the package version. I'd have to test this though, and that might be a while.

Maybe gate the converter behind this, and leave the old code as alternative for anyone that isn't on .NET 8 yet? Remember those tests serve as documentation, and having them work for fewer people could be a problem (at least it would be in my head, not sure if others disagree.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember those tests serve as documentation, and having them work for fewer people could be a problem (at least it would be in my head, not sure if others disagree.)

Yes, I agree, I think it's enough to provide a concrete example that people can use to transform different complex objects, so we shouldn't worry too much about whether it works in all versions,people will always have a way to make it work😁

@lahma
Copy link
Collaborator

lahma commented Mar 14, 2024

Please note that this way does not lead to optimal performance as it's still reflection based - and it's been before too also with Newtonsoft.Json, but if you would want optimal solution performance-wise then a custom ObjectWrapper should be created which would use known types instead of reflection when accessing properties.

@hyzx86
Copy link
Contributor Author

hyzx86 commented Mar 14, 2024

Thanks for the suggestion, I just don't know why this expression doesn't work with ObjectWrapper , I found out through debugging that it does differ from JsBoolean.False

var result2 = engine.Evaluate("!variables.falseValue");

@lahma lahma merged commit afdc082 into sebastienros:main Mar 14, 2024
3 checks passed
@hyzx86 hyzx86 changed the title Add test code to validate boolean vlue in the if condition. Add test code to validate boolean value in the if condition. Jun 13, 2024
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

3 participants