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

Fix #4066: Move Convertible#render_liquid to using render! #4077

Merged
merged 2 commits into from Oct 30, 2015

Conversation

Projects
None yet
5 participants
@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Oct 29, 2015

Contributor

Thanks! 👍

Contributor

fw42 commented Oct 29, 2015

Thanks! 👍

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Oct 29, 2015

Contributor

Should probably add a regression test for this

Contributor

fw42 commented Oct 29, 2015

Should probably add a regression test for this

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 29, 2015

Member

@fw42 great idea.

I found the errors in the templates helpful, too... But I guess terminal errors would be better because they'd break the build which is what we want when something goes wrong.

Member

parkr commented Oct 29, 2015

@fw42 great idea.

I found the errors in the templates helpful, too... But I guess terminal errors would be better because they'd break the build which is what we want when something goes wrong.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 29, 2015

Member

LGTM pending the regression test. A cucumber test will suffice.

Member

parkr commented Oct 29, 2015

LGTM pending the regression test. A cucumber test will suffice.

@envygeeks envygeeks added this to the 3.1 milestone Oct 29, 2015

@envygeeks envygeeks self-assigned this Oct 29, 2015

@parkr parkr added the fix label Oct 29, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 29, 2015

Contributor

I'll add the test after I finish up with my other pull requests.

Contributor

envygeeks commented Oct 29, 2015

I'll add the test after I finish up with my other pull requests.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Oct 29, 2015

Contributor

Cowabunga

Contributor

benbalter commented Oct 29, 2015

Cowabunga

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 29, 2015

Contributor

It took a while to write that test, it was crazy because I couldn't figure out why it wasn't getting output and it turned out there might be a slight bug in POSIX::Spawn::Child somewhere.

Contributor

envygeeks commented Oct 29, 2015

It took a while to write that test, it was crazy because I couldn't figure out why it wasn't getting output and it turned out there might be a slight bug in POSIX::Spawn::Child somewhere.

@envygeeks envygeeks modified the milestones: 3.0.1, 3.1 Oct 29, 2015

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Oct 29, 2015

Contributor

I would have just tested that render_liquid doesn't swallow exceptions, but I guess this works too

Contributor

fw42 commented Oct 29, 2015

I would have just tested that render_liquid doesn't swallow exceptions, but I guess this works too

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 30, 2015

Contributor

I considered that, but then I decided that we want the errors to bubble at the UX so tested it in Cucumber.

Contributor

envygeeks commented Oct 30, 2015

I considered that, but then I decided that we want the errors to bubble at the UX so tested it in Cucumber.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr
Member

parkr commented Oct 30, 2015

:shipit:

parkr added a commit that referenced this pull request Oct 30, 2015

@parkr parkr merged commit 528ec27 into master Oct 30, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@parkr parkr deleted the fix_render branch Oct 30, 2015

parkr added a commit that referenced this pull request Oct 30, 2015

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.