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

feat: add support for losslessOption #200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MangelMaxime
Copy link
Contributor

Add ability to support lossless option.

With the current implementation of Thoth.json, None or Some (Some None) are both encoded has null.

This PR introduce 2 new API:

  • lossyOption: is equivalent to the old option API

     // None
     null
     
     // Some None
     null
     
     // Some 42
     42
    
     // Some (Some 42)
     42
  • losslessOption which is able to differentiate the nested options

     // None
     {
         "$type": "option",
         "$case": "none"
     }
     
     // Some None
     {
         "$type": "option",
         "$case": "some",
         "$value": {
             "$type": "option",
             "$case": "none"
         }
     }
     
     // Some 42
     {
         "$type": "option",
         "$case": "some",
         "$value": 42
     }
    
     // Some (Some 42)
     {
         "$type": "option",
         "$case": "some",
         "$value": {
             "$type": "option",
             "$case": "some",
             "$value": 42
         }
     }

Warning

The old Decode.option and Encode.option will be completely removed. People, will need to manual path their code to use the lossyOption API.

I am removing the API completely because the API changes only impact the new version of Thoth.Json, so this is I believe the perfect moment to do it instead of introducing an Obsolete warning to be removed later on.

If you think differently please voice int

If we agree on the implementation, we will need to also change the object builder API to reflect this changes too.

@njlr
Copy link
Contributor

njlr commented Aug 5, 2024

Could the old Decode.option and Encode.option be left as-is? Encode.lossyOption is quite verbose!

@MangelMaxime
Copy link
Contributor Author

I agree that lossyOption and losslessOption are verbose but it forces people to think about what they want.

In an ideal world Decode.option would have always been equivalent to losslessOption because I think it makes more sense for it to work at any level of nested option.

I don't think I want to keep Decode.option because people will keep using it without knowing that the runtime representation is not safe?

Another solution I considered was to keep only Decode.option but make the decoder works with both the old and new representation making it kind of backward compatible. However, I can't do something similar for the encoder...

If people find it too lengthy, they can always create an alias so they can choose the default they prefer in they application?

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.

2 participants