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

Fix ToJson and FromJson liquid files for System.Text.Json #1339

Closed
wants to merge 1 commit into from

Conversation

unchase
Copy link
Contributor

@unchase unchase commented Mar 8, 2021

image

Now generates:

...
public string ToJson() 
{
    return System.Text.Json.JsonSerializer.Serialize(this, new System.Text.Json.JsonSerializerOptions(); var converters = new System.Text.Json.Serialization.JsonConverter[] { new Converter1() });
}
...

image


Fixed in this PR (need to check).

@unchase unchase changed the title Fix ToJson and FromJson liquid files for System.Text.Json [WIP] Fix ToJson and FromJson liquid files for System.Text.Json Mar 9, 2021
@unchase unchase changed the title [WIP] Fix ToJson and FromJson liquid files for System.Text.Json Fix ToJson and FromJson liquid files for System.Text.Json Mar 9, 2021
@unchase
Copy link
Contributor Author

unchase commented Mar 9, 2021

@RicoSuter ready for review

@RicoSuter
Copy link
Owner

RicoSuter commented Mar 9, 2021

In the end we should clean this up as I'm not a fan to parse/split etc. code in the templates..
but for now it's probably fine. What do you think?

/cc @jeremyVignelles

@jeremyVignelles
Copy link
Contributor

I might have missed something, but what's the rationale of this? why are there two instructions that you need to split?
Why should it be done there rather than in a property in the C# code?

@unchase
Copy link
Contributor Author

unchase commented Mar 9, 2021

@jeremyVignelles

Initially, the CSharpJsonSerializerGenerator.GenerateJsonSerializerParameterCode method was intended for the Newtonsoft.Json library and allows you to return the required code as a single line (along with initialization of custom converters).
When it became necessary to add support for System.Text.Json, there were two options, due to the fact that adding custom converters for System.Text.Json is different with Newtonsoft.Json and cannot be done in the initializer of the System.Text.Json.JsonSerializerOptions class (which allows Newtonsoft.Json.JsonSerializerSettings):

  1. Add everything in one line and then split this line into two parts (as implemented now).
  2. Refactor the CSharpJsonSerializerGenerator.GenerateForJsonLibrary method in such a way that backward compatibility is lost (due to the possible addition of a separate method that returns the addition of custom converters for System.Text.Json).

Thus, I chose the first option. Perhaps the issue can be solved in another way. If you have any ideas on how to do this, I would be grateful.

@jeremyVignelles
Copy link
Contributor

It seems that the CSharpJsonSerializerGenerator.GenerateJsonSerializerParameterCode is used for the purpose of setting the parameters to the Deserialize and Serialize methods. Why is there a ; var converters = there ?

Refactor the CSharpJsonSerializerGenerator.GenerateForJsonLibrary method in such a way that backward compatibility is lost (due to the possible addition of a separate method that returns the addition of custom converters for System.Text.Json).

I'd be OK for breaking backward compatibility to add a new Method/Property that would be specific to STJ

That said, I don't have the whole picture and that's still quite obscure for me, but making assumption on the shape of the generated code seems really rigid to me. What if I want to override the C# code to generate the converters my own way ?

@unchase
Copy link
Contributor Author

unchase commented Mar 9, 2021

Need to think about how to refactor the method to make it more flexible.

@unchase
Copy link
Contributor Author

unchase commented Mar 9, 2021

Related: #1341

@unchase unchase closed this Mar 9, 2021
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