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

Implement reflect.OutOf #39

Merged
merged 11 commits into from
Jun 11, 2021
Merged

Implement reflect.OutOf #39

merged 11 commits into from
Jun 11, 2021

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented Jun 2, 2021

Implement reflect.OutOf.

@kmoe kmoe mentioned this pull request Jun 6, 2021
6 tasks
@kmoe kmoe force-pushed the katy_reflect_outof branch from 3bc5ce5 to 4d00655 Compare June 8, 2021 21:22
kmoe and others added 8 commits June 10, 2021 21:41
Misc changes:
* Stub out support for maps
* Export the recursion functions so they can be tested from reflect_test
* Separate out our number types so they can be associated with different
  attr.Types (still not sure if this is a good idea or a mistake)
* Stop using `reflect.Value.Interface()`, because it forces us to assert
  a type, which will break on type aliases. Instead, use the
  `reflect.Value` methods that return strongly-typed primitives, making
  the reflect package handle the conversion for us.
* Call `tftypes.ValidateValue` before calling `tftypes.NewValue` so we
  error instead of panicking.
* Stop using `trueReflectValue` and handle things at every layer.
* Add support for floats.
* Reuse the `getStructTags` helper instead of reimplementing it
* Recurse into `OutOf` instead of reimplementing the switch inside
  `FromStruct`
* Accept an `interface{}` instead of a `reflect.Value`, letting callers
  ignore the involvement of the `reflect` package. Also, have our
  helpers take strongly-typed parameters instead of `reflect.Value`s,
  simplifying them.
Separate out into/outof into their own files, for ease of reading.

Stop pulling types out of OutOfOptions, instead preferring to pass in
the complete type information ahead of time.

Add support for slices, maps, pointers.

Stub out and TODO support for interfaces.
@paddycarver paddycarver marked this pull request as ready for review June 11, 2021 06:15
@paddycarver
Copy link
Contributor

I think this is now ready for review. It has a good amount of test coverage, and seems to be able to do what we want from it, which is generating an attr.Value from an arbitrary Go type and a schema. Map support still needs testing, and tuples and sets still need handled, but we can do that as those types land, just like reflect.Into.

@paddycarver paddycarver requested a review from a team June 11, 2021 06:16
@paddycarver paddycarver added this to the v0.1.0 milestone Jun 11, 2021
internal/reflect/options.go Outdated Show resolved Hide resolved
internal/reflect/number.go Outdated Show resolved Hide resolved
@paddycarver
Copy link
Contributor

Ready for another review pass, I think.

@kmoe
Copy link
Member Author

kmoe commented Jun 11, 2021

Excellent! I hereby approve the latest changes (I can't mark the PR approved as I'm the author).

@kmoe kmoe merged commit ec0146a into main Jun 11, 2021
@kmoe kmoe deleted the katy_reflect_outof branch June 11, 2021 15:59
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants