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

Introduce ApplyWithOptions #118

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

vassilvk
Copy link
Contributor

@vassilvk vassilvk commented Oct 2, 2020

  • Implement ApplyWithOptions
  • Implement ApplyWithOptions unit tests
  • Fix compareJSON to allow comparison of array containers
  • Fix previously incorrect "copy" operation tests

Closes #117.

* Implement ApplyWithOptions
* Implement ApplyWithOptions unit tests
* Fix compareJSON to allow comparison of array containers
* Fix previously incorrect "copy" operation tests
@vassilvk
Copy link
Contributor Author

Hi @evanphx,

Any feedback?

@evanphx
Copy link
Owner

evanphx commented Oct 14, 2020

Hi! Sorry for the delay. I think my main feedback is that we should pass the opts struct down into the methods that have these options, rather than reading it at the top as arguments. The reason here is I can see the next person come along and want to add another option and they'll end up adding another argument unless they can just use the opts struct directly.

Does that make sense?

@vassilvk
Copy link
Contributor Author

vassilvk commented Oct 14, 2020

Hi - thanks for the feedback.
Yes, it does make sense and I did flip back and forth on that one quite a bit (my first implementation passed down the options).

The main reason I chose to pass explicit parameters was that technically this is cleaner as each private method gets to maintain its independence from the bigger picture (Apply).

That said, I know I’m that “next person” 🙂 - I need yet another option for more “forgiving” add operations.

I will switch to passing the options struct to the private methods.

@vassilvk
Copy link
Contributor Author

@evanphx - I switched to passing options to private methods.
I also updated README to include the new ApplyWithOptions method and new AllowMissingPathOnRemove option.
Please review.

@evanphx
Copy link
Owner

evanphx commented Nov 3, 2020

Thanks a bunch @vassilvk!

@evanphx evanphx merged commit bce379e into evanphx:master Nov 3, 2020
@vassilvk vassilvk deleted the 117-introduce-applywithoptions branch November 25, 2020 22:00
@AkihiroSuda
Copy link

Would it be possible to create a new release with this? 🙏

@evanphx
Copy link
Owner

evanphx commented Jan 6, 2021

@AkihiroSuda This went out in v5.2.0. Sorry for the delay!

@AkihiroSuda
Copy link

Thanks!

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.

Introduce ApplyWithOptions
3 participants