feat: use isReflectNil instead of isEmptyValue to gate Transformers #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Not sure what's going on with travis, as same tests are passing locally :\ |
|
Can you dig deeper to see if there are versions that are not matching up? |
|
I added a build matrix for various releases of go and it seems this works with >= 1.13 I see 1.13 introduced some changes to the reflect package, maybe that's related |
|
I'm not familiar with this codebase so can't review. I'll trust you with this one for now.
|
Mergo handles merges with zero-value fields as follows: 1. dst: true src: false -> dst: true 2. dst: nil src: false -> dst: false 3. dst: nil src: true -> dst: true 4. dst: false src: true -> dst: true In some cases (darccio#166) it may be desirable to change this default behaviour using a custom Transformer, for example to not overwrite 'dst' when it's already set to 'false' (or '0' etc). Unfortunately, in such cases (when dst holds a zero-value) no Transformers can be applied due to the way these are gated (darccio#166). Here we are changing how transformers are gated by using the isReflectNil function insted of isEmptyValue. Note that: - this change doesn't affect the default behaviour of mergo, as it only affects how transformers are gated - since custom transformers need to match data types in order to execute, this shouldn't affect any existing transformers either
GGabriele
force-pushed
the
gating_transformers
branch
from
7d606d2
to
32bd3e7
Compare
Done! |
|
Go ahead and merge and do a release. |
GGabriele
added a commit
to Kong/go-kong
that referenced
this pull request
Feb 2, 2022
We recently forked and patched mergo (Kong/mergo#1) to improve the way we can handle zero-value fields with defaults values. Currently, if an entity field is set with a zero-value (e.g. a target with weight=0, a route with strip_path=false etc.), we will overwrite it when merging it with its default if this is not a zero-value itself. This update the mergo dependence and add a custom transformer to support such cases as well. Also, add few test cases with zero-value fields.
rainest
pushed a commit
to Kong/go-kong
that referenced
this pull request
Feb 2, 2022
We recently forked and patched mergo (Kong/mergo#1) to improve the way we can handle zero-value fields with defaults values. Currently, if an entity field is set with a zero-value (e.g. a target with weight=0, a route with strip_path=false etc.), we will overwrite it when merging it with its default if this is not a zero-value itself. This update the mergo dependence and add a custom transformer to support such cases as well. Also, add few test cases with zero-value fields.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Original upstream PR with reasoning behind this patch: darccio#203
Couple of additional comments:
mergo(see below), but it only affects how transformers are gated