search

rails / jbuilder

Jbuilder: generate JSON objects with a Builder-style DSL
MIT License
4.34k stars 440 forks source link

Fix passing object with partials w/o locals (#378). #435

Closed siegfault closed 6 years ago

siegfault commented 6 years ago

This uses @SGospodinov's specs written on the report from @yusufali2205. I ran into this issue as well today and, while I don't think this necessarily the final solution, I wanted to move the conversation along.

Previous behavior: Passing objects to partial fails if the below format is used:

json.comments @post.comments, partial: 'comments/comment', as: :comment, show: false

It succeeds if locals option is used:

json.comments @post.comments, partial: 'comments/comment', locals: { show: false }, as: :comment

Swapping to options is a change to revitalize the conversation. I'm not sure it's a permanent solution.

rails-bot commented 6 years ago

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

siegfault commented 6 years ago

@pixeltrix would love any thoughts or advice on this one that you can provide! Thanks!

siegfault commented 6 years ago

@yusufali2205 just pushed a change that highlights and seems to fix the concern you had. I don't think it's a good solution for the reasons I mentioned in the commit that I'll copy here. Definitely looking for advice!

This is not a good solution because it puts all of options, related to locals or not, into the locals section of options. Will want advice for a better solution, but this does make the spec pass.

siegfault commented 6 years ago

I think this is now in a better place and ready for review @yusufali2205 / @pixeltrix. Happy to make any changes and would love any feedback. Thanks!

siegfault commented 6 years ago

@pixeltrix let me know if you have any advice or if you want me to ping someone else. Thanks!

siegfault commented 6 years ago

@rwz pinging you since it looks like you were getting assigned PRs before @pixeltrix. Any ideas on who I should pull in to take alook at this? Thanks!

@yusufali2205 also pinging you since you originally reported the issue and reviewed this earlier. This seems to fix the issue for me. Mind making sure that it also does for you? Thanks!

siegfault commented 6 years ago

Hi @rafaelfranca! Pinging you since you seem to be getting assigned on PRs now (though it's noting that the repo may be misconfigured). Are you the right person to take a look? If not, do you know who would be? Either way, thanks!

siegfault commented 6 years ago

Hi @dhh! Pinging you since you merged a PR in the repo last week. Not necessarily asking for review unless you want to take a look, but since the repo is misconfigured and I haven't gotten much traction here, was wondering if you had suggestions for who to reach out to. Thanks!

dhh commented 6 years ago

Hey, sorry you haven't gotten any traction on this yet. Thank for you for keep pushing!

This looks correct to me. Merging. Thanks again!

siegfault commented 6 years ago

Thanks!

yusufali2205 commented 5 years ago

Sorry for not replying to the messages, I didn't check on this since long time. Thanks @siegfault for fixing this.