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

JsonSerializerOptionsAnalyzer should also mention .Default #44

Open
bartelink opened this issue Dec 12, 2023 · 3 comments
Open

JsonSerializerOptionsAnalyzer should also mention .Default #44

bartelink opened this issue Dec 12, 2023 · 3 comments

Comments

@bartelink
Copy link

bartelink commented Dec 12, 2023

While I don't disagree with the advice in https://g-research.github.io/fsharp-analyzers/analyzers/JsonSerializerOptionsAnalyzer.html#JsonSerializerOptionsAnalyzer, I'd add a mention of JsonSerializerOptions.Default,

Going further, I believe the better overall pattern is using a Serdes wrapper with a mandatory options arg a la https://github.com/jet/FsCodec/blob/master/src/FsCodec.SystemTextJson/Serdes.fs#L7, but I can appreciate that shoehorning that advice/guidance into an Analyzer might be a stretch!

(Aside: this can be taken even futher by having a lazily-implemented .Default associated with each suite of Options a la
https://github.com/jet/FsCodec/blob/master/src/FsCodec.SystemTextJson/Options.fs#L14 and following the general pattern outlined in https://github.com/jet/fscodec?tab=readme-ov-file#examples-of-using-serdes-to-define-a-contract )

@nojaf
Copy link
Contributor

nojaf commented Dec 12, 2023

Hello,

While I don't disagree with the advice

This is interesting. I can't say that we based the analyzer on anything other than the feedback of the developers over at G-Research. Could you elaborate on why you disagree with this guidance? Your perspective would be insightful.

JsonSerializerOptions.Default seems like a good addition to mention indeed.

@bartelink
Copy link
Author

I DONT disagree with the guidance, but, depending on how it's worded, it might prompt people to skip a potentially simpler solution.

  • It is always good advice to not assume that the options are cheap to gen
  • And hence that caching it is useful

My point is solely that the most common resolution for the literal example provided (creating default settings via the default ctor) already has an even better solution - use the in-the-box .Default property added in STJ V7.

The rest is just me saying "and if you have any Options builders that vary from STJ's Options.Default, then consider following that pattern in your impl:

  • have a default ctoe
  • put a .Default property alongside your builder method that caches a default-constructed set of options so the work can be cached cleanly

@nojaf
Copy link
Contributor

nojaf commented Dec 12, 2023

Whoops, I misread that. Thanks for elaborating.

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

No branches or pull requests

2 participants