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

Handle IConvertible case to return specific type FluidValues #612

Merged

Conversation

atthevergeof
Copy link
Contributor

@atthevergeof atthevergeof commented Dec 8, 2023

fixes #609

The JValue class does implement IConvertible, but in Fluid.Create, when System.Type.GetTypeCode() is called with JValue type in the argument, it returns TypeCode.Object. And under this case, it was going to the IFormattable case and being converted to string regardless of the underlying data type. But when JValue is cast to IConvertible and then the GetTypeCode() method is called on the object, it does return the correct type code corresponding to the underlying value.

So to resolve the issue #609, I have moved the IConvertible case above IFormattable, and also changed its implementation to extract the underlying value and call the Create method again with that value. To prevent any infinite recursion, I have explicitly handled the TypeCode.Object case by returning the string value for it.

@atthevergeof atthevergeof reopened this Dec 8, 2023
@lahma
Copy link
Collaborator

lahma commented Dec 8, 2023

You probably want to add some test cases to demonstrate what you have fixed and also to ensure nobody breaks the behavior you are after later on.

@atthevergeof atthevergeof force-pushed the fix/fluid-value-rectification branch from 38bed35 to bb42949 Compare December 8, 2023 17:32
@atthevergeof
Copy link
Contributor Author

Added a test case to validate the change

Skip `Convert` usage that is boxing all value types
@sebastienros sebastienros merged commit 81ab72f into sebastienros:main Dec 8, 2023
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.

Using json filter on a JObject coverts all values to string
3 participants