Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Add a value hook func #183

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Add a value hook func #183

merged 1 commit into from
Nov 26, 2020

Conversation

camdencheek
Copy link
Contributor

@camdencheek camdencheek commented Mar 28, 2020

Overview

This commit adds a third hook func type which has access to the reflect.Value representations of both the from and the to objects. This gives all the same power of the existing hook functions (types, kinds, and data can all be retrieved from values), but with the additional benefit of being able to modify the to object.

Change scope

This change is largely backwards compatible. The only change of the public API (other than adding a new hook type) is a change in the signature of DecodeHookExec. If that's a sticking point, I think there may be a way around it, but I haven't looked closely at it.

Motivation

Exposing the value of to opens up some really powerful patterns such as:

Overall, I think if this is done right, it could really open a lot of possibility for more powerful hook funcs. The API works well, but I plan to play around with it a bit more to make sure it's as flexible as possible. I'm obviously open to any feedback

This commit adds a third hook func type which has access to the
reflect.Value representations of both the `from` and the `to`
objects. This gives all the same power of the existing hook functions
(types, kinds, and data can all be retrieved from values), but
with the additional benefit of being able to modify the `to` object.

This change is largely backwards compatible. The only change of the
public API (other than adding a new hook type) is a change in the
signature of `DecodeHookExec`.
@smyrman
Copy link

smyrman commented May 15, 2020

Alternate suggestion in #192

@mitchellh
Copy link
Owner

I like this! I like that it fits into the existing style of decoder hooks.

As much as I'd rather not break BC, the DecodeHookExec function I've never seen used and probably shouldn't have been publicly exposed in the first place. I think I only did that for testing helper purposes. I'd be okay with breaking API in this PR for that specific API.

I'll merge this as-is, but let me know if you wanted to make other changes to it. Thank you for the tests.

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.

3 participants