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

Dont render blank blocks #218

Merged
merged 10 commits into from
Jul 2, 2013
Merged

Dont render blank blocks #218

merged 10 commits into from
Jul 2, 2013

Conversation

fw42
Copy link
Contributor

@fw42 fw42 commented Jun 27, 2013

Very similar to #217. This one also has lots of tests. We should coordinate better :-)

@hornairs, @boourns, @csfrancis, @camilo, @dylanahsmith, @arthurnn

@fw42 fw42 mentioned this pull request Jun 27, 2013
@fw42
Copy link
Contributor Author

fw42 commented Jun 27, 2013

Added a few more tests/cases. Reading the tests is the easiest way to understand what effect this is going to have I think. I'm sure I forgot something. Somebody (other than me) should take a look :-)

Conflicts:
	lib/liquid/tags/cycle.rb
	lib/liquid/tags/increment.rb
@csfrancis
Copy link

Here are benchmarks from my machine for this branch:

[scottifyBookPro] liquid (master) $ rake benchmark:run
/opt/boxen/rbenv/versions/1.9.3-p385-perf/bin/ruby ./performance/benchmark.rb
Rehearsal ------------------------------------------------
parse:         3.160000   0.050000   3.210000 (  3.210458)
parse & run:   7.110000   0.060000   7.170000 (  7.169069)
-------------------------------------- total: 10.380000sec

                   user     system      total        real
parse:         3.150000   0.030000   3.180000 (  3.185234)
parse & run:   6.940000   0.030000   6.970000 (  6.966688)
[scottifyBookPro] liquid (master) $ rake benchmark:run
/opt/boxen/rbenv/versions/1.9.3-p385-perf/bin/ruby ./performance/benchmark.rb
Rehearsal ------------------------------------------------
parse:         3.030000   0.050000   3.080000 (  3.084493)
parse & run:   6.980000   0.060000   7.040000 (  7.037003)
-------------------------------------- total: 10.120000sec

                   user     system      total        real
parse:         3.020000   0.000000   3.020000 (  3.023140)
parse & run:   6.920000   0.040000   6.960000 (  6.958436)
[scottifyBookPro] liquid (master) $ git checkout dont_render_blank_blocks
Switched to branch 'dont_render_blank_blocks'
[scottifyBookPro] liquid (dont_render_blank_blocks) $ rake benchmark:run
/opt/boxen/rbenv/versions/1.9.3-p385-perf/bin/ruby ./performance/benchmark.rb
Rehearsal ------------------------------------------------
parse:         3.170000   0.050000   3.220000 (  3.222177)
parse & run:   7.180000   0.030000   7.210000 (  7.200545)
-------------------------------------- total: 10.430000sec

                   user     system      total        real
parse:         3.130000   0.020000   3.150000 (  3.146572)
parse & run:   7.080000   0.020000   7.100000 (  7.101301)
[scottifyBookPro] liquid (dont_render_blank_blocks) $ rake benchmark:run
/opt/boxen/rbenv/versions/1.9.3-p385-perf/bin/ruby ./performance/benchmark.rb
Rehearsal ------------------------------------------------
parse:         3.070000   0.040000   3.110000 (  3.114575)
parse & run:   7.000000   0.030000   7.030000 (  7.026592)
-------------------------------------- total: 10.140000sec

                   user     system      total        real
parse:         3.050000   0.010000   3.060000 (  3.061203)
parse & run:   6.960000   0.040000   7.000000 (  6.995924)

Results are similar to #217 (comment)

@ghost ghost assigned fw42 Jul 2, 2013
@tobi
Copy link
Member

tobi commented Jul 2, 2013

I think that's a nice solution.

fw42 added a commit that referenced this pull request Jul 2, 2013
@fw42 fw42 merged commit 4a103a9 into master Jul 2, 2013
@fw42 fw42 deleted the dont_render_blank_blocks branch July 2, 2013 22:15
fw42 pushed a commit that referenced this pull request Jul 13, 2013
This reverts commit 4a103a9, reversing
changes made to 5c5e7de.

Conflicts:
	lib/liquid/block.rb
	test/liquid/blank_test.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants