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

Two errors in custom merge #55

Closed
atz opened this issue Aug 24, 2016 · 3 comments
Closed

Two errors in custom merge #55

atz opened this issue Aug 24, 2016 · 3 comments

Comments

@atz
Copy link
Member

atz commented Aug 24, 2016

Bad check at:
https://github.com/projectblacklight/blacklight_advanced_search/blob/master/lib/blacklight_advanced_search.rb#L30

if new_value.respond_to?(:blank) && new.blank?

:blank vs. :blank? and new_value vs. new

Looks like the new/new_value error was introduced here: 6d3b49a#diff-4961a18d7c088f36dc4f670c73ae74a6R30

And the other in 2011 or before (so far back, I don’t care anymore). Got test coverage?

@atz
Copy link
Member Author

atz commented Aug 26, 2016

Maybe time to undo that whole method now that BL doesn't have to support rails3?
projectblacklight/blacklight#827

@atz
Copy link
Member Author

atz commented Aug 26, 2016

Note: when I fix this error, the behavior further diverges from Rails deep_merge because

false.blank? ==> true
true.blank? ==> false

So any boolean false values in the overlaying hash are ignored. But when set to true the same values are enforced. This doesn't make any sense, but Rails has chosen.

@mjgiarlo
Copy link
Member

👀

atz added a commit that referenced this issue Aug 27, 2016
Fixes #55.

Use mainline Rails `deep_merge` with a block.

Massively more robust testing, including running the deep_merge! tests
with HashWithIndifferentAccess objects to allay concerns about that class'
incompatibility.

Added docs.
atz added a commit that referenced this issue Aug 29, 2016
Fixes #55.

Use mainline Rails `deep_merge` with a block.

Massively more robust testing, including running the deep_merge! tests
with HashWithIndifferentAccess objects to allay concerns about that class'
incompatibility.

Added docs.
atz added a commit that referenced this issue Aug 29, 2016
Fixes #55.

Use mainline Rails `deep_merge` with a block.

Massively more robust testing, including running the deep_merge! tests
with HashWithIndifferentAccess objects to allay concerns about that class'
incompatibility.

Added docs.
atz added a commit that referenced this issue Aug 29, 2016
Fixes #55.

Use mainline Rails `deep_merge` with a block.

Massively more robust testing, including running the deep_merge! tests
with HashWithIndifferentAccess objects to allay concerns about that class'
incompatibility.

Added docs.
cbeer pushed a commit that referenced this issue Aug 29, 2016
Fixes #55.

Use mainline Rails `deep_merge` with a block.

Massively more robust testing, including running the deep_merge! tests
with HashWithIndifferentAccess objects to allay concerns about that class'
incompatibility.

Added docs.
@cbeer cbeer closed this as completed in #59 Aug 29, 2016
cbeer pushed a commit that referenced this issue Sep 2, 2016
Fixes #55.

Use mainline Rails `deep_merge` with a block.

Massively more robust testing, including running the deep_merge! tests
with HashWithIndifferentAccess objects to allay concerns about that class'
incompatibility.

Added docs.
cbeer pushed a commit that referenced this issue Jun 8, 2017
Fixes #55.

Use mainline Rails `deep_merge` with a block.

Massively more robust testing, including running the deep_merge! tests
with HashWithIndifferentAccess objects to allay concerns about that class'
incompatibility.

Added docs.
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

No branches or pull requests

2 participants