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.

Sign up for GitHub

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

Jump to bottom

fix: gate transformers on valid non-nil destinations #211

Merged
merged 3 commits into from
May 24, 2022
Merged

fix: gate transformers on valid non-nil destinations #211

merged 3 commits into from
May 24, 2022

Commits on May 18, 2022

  1. fix: gate transformers on valid non-nil destinations 

    This builds on darccio#203 which attempted to provide a more flexible gating to
running transformers. However upon testing darccio#203 in my own environment, I
ran into the first panic listed below.

There are a variety of errors that can happen when trying to run
reflection on zero values:

2 just from my testing of this PR
```
        panic: reflect: call of reflect.Value.Type on zero Value

        panic: reflect: call of reflect.Value.FieldByName on zero Value
```
The panic specifically calls out zero values, but it's actual a more
specific set of values which is covered by `reflect.IsValid`.

I attempted to replace the check with `reflect.IsZero`, which ends up
being too restrictive and breaks existing tests. I also attempted to
solely use `reflect.IsZero` which is not restrictive enough and cause
the later panic above in the tests. Thus I arrived on the combination in
this PR which seems to strike the "right" balance.
    Zaq? Wiedmann committed May 18, 2022
    Configuration menu
    Copy the full SHA
    ab6b270 View commit details
    Browse the repository at this point in the history

  2. add test for keeping zero values with transformer

    Zaq? Wiedmann committed May 18, 2022
    Configuration menu
    Copy the full SHA
    4bed36e View commit details
    Browse the repository at this point in the history

  3. add test for deepMerge panic

    Zaq? Wiedmann committed May 18, 2022
    Configuration menu
    Copy the full SHA
    8109749 View commit details
    Browse the repository at this point in the history