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

deep merge blocks #286

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

deep merge blocks #286

wants to merge 1 commit into from

Conversation

odedniv
Copy link

@odedniv odedniv commented Jul 16, 2015

This works for hashes and arrays using the already existing Jbuilder#_merge_values method.

The only syntax not merging now is when you change the value in the same block:

puts Jbuilder.encode { |x| x.a { x.b [1] }; x.a { x.b [2] } } # Works fine, now
=> {"a":{"b":[1,2]}}
puts Jbuilder.encode { |x| x.a { x.b(c: 1) }; x.a { x.b(d: 2) } } # Also works fine
=> {"a":{"b":{"c":1,"d":2}}}
puts Jbuilder.encode { |x| x.a { x.b [1]; x.b [2] } } # Still doesn't work
=> {"a":{"b":[2]}}

Tried to modify Jbuilder#_set_value to fix it, and it caused unexpected problems. Still worth the merge.

@odedniv
Copy link
Author

odedniv commented Jul 16, 2015

The only backward incompatibility I can think of is if the user used to accidentally overwrite the same inner value.

Previously it would just replace it (whether it's an Array, Hash or anything else), now it will append if Array or Hash, and crash if anything else (Can't merge X with Y).

But this was a weird behavior because if he would do the same thing inside the same block it would act differently (like it would now).

@odedniv
Copy link
Author

odedniv commented Dec 30, 2015

Hmm, bump? This is very useful and I don't think it's a very hard PR to consume...

@rwz
Copy link
Collaborator

rwz commented Dec 30, 2015

I don't get the usecase for that. Why would someone set the same value in the same block twice?

Also, I'd expect it to overwrite the previous value instead of making an attempt to merge.

@odedniv
Copy link
Author

odedniv commented Dec 30, 2015

Well I happen to use Jbuilder for building complex JSONs (not just views), specifically for Elasticsearch.

The code generating the JSON is split into many functions in different models that all contribute to the same big JSON sent to the API, so it is very useful for the different code areas to add more objects to the (maybe) already existing array.

Even more specifically if you're still not convinced of the use case, a JSON of "filters" to search by:

# This is actually an interface with different implementations
class Filter
  def build_search(search)
    # adding to filters array
    search.filters do
      search.array!(1.times) do
        search.match x: @x
      end
    end
    # adding to sort array
    search.sort do
      search.array!(1.times) do
        search.field :x
      end
    end
  end
end

filters = [Filter.new, ...]

Jbuilder.encode do |search|
  filters.each do |filter|
    filter.build_search(search)
  end
end

I guess I can build the array outside and have a lot of has_filter?, has_sort? functions, but that would make the infrastructure (the outer function) as well as the classes themselves much more complex.

Also, the current implementation is inconsistent as sometimes it would merge and sometimes it wouldn't (see original comment). Hope that helped explaining my point.

@odedniv
Copy link
Author

odedniv commented May 21, 2016

@rwz any reason not to merge this?

@odedniv
Copy link
Author

odedniv commented Jul 18, 2016

So... I'm guessing this is not going in?

FYI #332 (@melnikaite) also wanted it.

@odedniv
Copy link
Author

odedniv commented May 16, 2017

Why is this not pulled? It makes complete sense.

(Calling a seemingly active collaborator) @rafaelfranca what do you think?

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.

None yet

2 participants