-
Notifications
You must be signed in to change notification settings - Fork 440
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
base: main
Are you sure you want to change the base?
deep merge blocks #286
Conversation
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 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). |
Hmm, bump? This is very useful and I don't think it's a very hard PR to consume... |
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. |
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 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. |
@rwz any reason not to merge this? |
So... I'm guessing this is not going in? FYI #332 (@melnikaite) also wanted it. |
Why is this not pulled? It makes complete sense. (Calling a seemingly active collaborator) @rafaelfranca what do you think? |
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:
Tried to modify
Jbuilder#_set_value
to fix it, and it caused unexpected problems. Still worth the merge.