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

Named type converters #573

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Named type converters #573

merged 1 commit into from
Sep 17, 2024

Conversation

hannahhoward
Copy link
Collaborator

Goals

Currently, the bindnode API enables custom conversions, keyed on go types.

This PR enables a new way to key custom conversions: the type name in a schema.

Why is this useful?

  • First it enables using standard go types for custom conversions, rather than needing to create a named go type every time you want a converter
  • More interestingly, it enables using interface types within structs, lists, and maps -- since the conversion is keyed on the schema type name, the go type isn't needed to setup a custom conversion, and then the convert can create types that satisfy the interface -- this is super useful for more flexible go structs.

Implementation

  • For compatibility, this keeps all typed custom converter APIs working, with schema named converters taking preference. This does mean that the config type must become a struct, and therefore a *config where it is passed
  • The schema type name is passed to every call for converterFor & converterForType. Not sure what performance implications this may have (mostly in lookups)
  • Wrote a test demonstrating equivalent functioning working with named converters, and a test showing how to use it with interfaces (here used to define a closed union type, something I find is useful to represent often)

@hannahhoward hannahhoward requested a review from rvagg September 17, 2024 00:50
allow custom converters based off schema name, rather than go type -- this is particularly useful
for enabling interface members of structs to be used with bindnode
@hannahhoward hannahhoward force-pushed the feat/named-type-converter branch from 9065eb8 to fed71e8 Compare September 17, 2024 00:52
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

cool

@rvagg
Copy link
Member

rvagg commented Sep 17, 2024

windows ci failures are known flaky garbage, build output "ipldsch_minima.exe" already exists and is not an object file

@rvagg rvagg closed this Sep 17, 2024
@rvagg rvagg reopened this Sep 17, 2024
@hannahhoward hannahhoward merged commit 6148356 into master Sep 17, 2024
22 of 24 checks passed
@hannahhoward hannahhoward deleted the feat/named-type-converter branch September 17, 2024 22:32
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