Skip to content

Use a Merge function, to make merging of >2 inputs clearer#1872

Merged
j16r merged 4 commits intodevelopfrom
chore/clearer_parameter_merging
Nov 11, 2019
Merged

Use a Merge function, to make merging of >2 inputs clearer#1872
j16r merged 4 commits intodevelopfrom
chore/clearer_parameter_merging

Conversation

@j16r
Copy link
Copy Markdown
Contributor

@j16r j16r commented Nov 7, 2019

This is a small change to JSON#Merge which lifts it out into a function, rationale:

  1. As an isolated function it's clearer (to me) that this returns a copy and doesn't mutate anything
  2. The original used Map() which potentially returns a the underlying data structure of the object, which could mean Merge actually mutates the caller if a resize happens.
  3. Merge is now variadic, so merging in the RunManager becomes a bit clearer: models.Merge(previousTaskInput, currentTaskRun.Result.Data, run.Overrides) which makes it more obvious what is happening.
  4. Reduced error checking, only turning the result back into gjson encounters an error signature, which only needs to happen once for any number of merge operations.

@j16r j16r force-pushed the chore/clearer_parameter_merging branch 3 times, most recently from 23d4806 to bdd3b7f Compare November 11, 2019 16:41
@felder-cl felder-cl self-assigned this Nov 11, 2019
@j16r j16r force-pushed the chore/clearer_parameter_merging branch from bdd3b7f to 95e4c25 Compare November 11, 2019 16:59
@j16r j16r requested a review from coventry November 11, 2019 17:05
coventry
coventry previously approved these changes Nov 11, 2019
Copy link
Copy Markdown
Contributor

@coventry coventry left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread core/store/orm/orm_test.go Outdated
@j16r j16r force-pushed the chore/clearer_parameter_merging branch from 95e4c25 to 737cc6a Compare November 11, 2019 20:19
@j16r j16r merged commit 567eb78 into develop Nov 11, 2019
@j16r j16r deleted the chore/clearer_parameter_merging branch November 11, 2019 20:35
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.

3 participants