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

[wip] [experimental] New type and converter for expandable fields #1370

Closed
wants to merge 1 commit into from

Conversation

ob-stripe
Copy link
Contributor

Still very experimental, and the code is downright awful at the moment, but here's the gist of it:

  • adds a new ExpandableField<> generic class used to replace the previous StringOrObject<>.Map method. The class is simply a container for two properties:
    • a string (Id)
    • an instance of the type param (ExpandedObject)
  • adds an ExpandableFieldConverter custom converter class to deserialize ExpandableFields. The converter:
    • creates an instance of ExpandableField
    • if the JSON is a string, fills Id with the string and sets ExpandedObject to null
    • if the JSON is an object:
      • if the param type is an interface (i.e. if the type is a polymorphic resource), deserializes the JSON with the proper custom converter
      • if the param type is not an interface (i.e. it's a concrete class), directly deserializes the JSON into an instance of the type
      • sets ExpandableField to the deserialized instance and Id to the value of the Id property of the deserializes instance
      • if the deserialization fails for some reason (e.g. because of a new polymorphic type that the library doesn't know about), make a last attempt at manually extracting the id property of the raw JSON object
    • returns the filled ExpandableField instance

Sort of works, but we'll also need to add a custom deserializer for StripeList to handle polymorphic types there as well.

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

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

Okay so in principle, I really like where this is going and I think it would work. I don't think the code is especially awful either.

Some feedback that might help

  • I would love to see a test for serialization/deserialization to ensure this continues to work as devs expect
  • I wonder if there's value in only ever supporting the Expanded object and not have the customer get/setter for the id/object. Basically do what we did in Go and less what we did in Java if that makes sense
  • I'm curious how hard it will be to remember to changer the converter for future interfaces. Is it easy to add a test for this? If not, I worry that we really on tribal knowledge.

@ob-stripe
Copy link
Contributor Author

I would love to see a test for serialization/deserialization to ensure this continues to work as devs expect

Yep, definitely on my TODO list.

I wonder if there's value in only ever supporting the Expanded object and not have the customer get/setter for the id/object. Basically do what we did in Go and less what we did in Java if that makes sense

I'm on the fence about this, I kind of like that those custom getters help keep a ~1:1 mapping between the library's class structure and the API's structure. If we only exposed the ExpandableField attribute that would add a virtual layer that only exists in the library. Not sure it's that big a deal though.

I'm curious how hard it will be to remember to changer the converter for future interfaces. Is it easy to add a test for this? If not, I worry that we really on tribal knowledge.

Not sure this is easily (or even hardly 😛) testable. I think it's on us to keep in mind that handling polymorphic resource is one of the more fragile parts of the library and we should write thorough tests when such resources are added. I'll see if I can come up with something automated though.

@ob-stripe ob-stripe force-pushed the ob-new-exp-field branch 3 times, most recently from e8cce60 to 4361d46 Compare November 7, 2018 17:32
@ob-stripe ob-stripe force-pushed the remi-add-created-converter-back branch from 766e8f6 to b5180e7 Compare November 12, 2018 15:52
@ob-stripe
Copy link
Contributor Author

Superseded by #1396.

(I might still introduce an ExpandableField<> generic in a future PR as I think it's cleaner than the existing StringOrObject<>.Map trick, but trying to minimize changes for now.)

@ob-stripe ob-stripe closed this Nov 16, 2018
@ob-stripe ob-stripe deleted the ob-new-exp-field branch January 7, 2019 22:22
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