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

Proposal: remove :ugly option #894

Merged
merged 12 commits into from Feb 14, 2017
Merged

Proposal: remove :ugly option #894

merged 12 commits into from Feb 14, 2017

Conversation

@teeparham
Copy link
Member

@teeparham teeparham commented Feb 9, 2017

I propose we remove the :ugly option. Haml should render all output as ugly, and never attempt to format pretty HTML. All modern browsers automatically pretty-format HTML. All modern text editors have tools to pretty-format HTML. There is no longer a need for Haml to do this.

The benefits are speed and cleaner code.

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Feb 2, 2017

Strong +1 for this. I have some reasons for it.

  1. I want to develop applications with exactly the same behavior as production. Sometimes spacing on browser is different between pretty and ugly modes. Even though this is not a feature for production users, it's harmful for developers.
  2. Personally I use Chrome's inspector to browse HTML elements and never see a raw rendering result.
  3. Of course, as you mentiond, for maintenance cost. Pretty mode must be done on runtime, considering its behavior. It means not only we need to maintain two modes but also we can't migrate half of implementation to Temple.

Production applications are already ugly mode. Is there any use case we should care about template engine's rendering result?

@amatsuda
Copy link
Member

@amatsuda amatsuda commented Feb 2, 2017

:+10000: from me!

Removing the ugly mode is the first thing I did when I made my own Haml alternative (which was never publicly released).

If no one disagrees, we can just drop the feature in the next major release. It's nonsense to print a deprecation warning for ugly mode users, so there's no way we can announce the removal beforehand.

Thank you @teeparham for bringing this topic in a perfect timing!

teeparham added a commit to teeparham/haml-spec that referenced this pull request Feb 2, 2017
@teeparham
Copy link
Member Author

@teeparham teeparham commented Feb 2, 2017

I pushed a rm-ugly branch with some work in progress. Getting the tests passing will take some more work.

Also we need to merge haml/haml-spec#13 & update the haml-spec project (it's a subproject of haml).

@amatsuda
Copy link
Member

@amatsuda amatsuda commented Feb 2, 2017

@teeparham 👍

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Feb 2, 2017

👍

I hope this branch will be merged soon!

teeparham added a commit to teeparham/haml-spec that referenced this pull request Feb 2, 2017
@teeparham
Copy link
Member Author

@teeparham teeparham commented Feb 9, 2017

This is almost ready. It has been rebased on the latest master. Thanks to @k0kubun for making this much easier by separating the pretty tests!

There are still some failing tests:

  1. The tests in HamlTest (based on data in haml-spec) should pass once we merge & update haml/haml-spec#13.
  2. I am not sure about the escaping in a 2 tests in EngineTest - help!
  3. I am not sure why HelperTest#test_error_return_line gives the wrong line - help!
@k0kubun
Copy link
Member

@k0kubun k0kubun commented Feb 9, 2017

I am not sure why HelperTest#test_error_return_line gives the wrong line - help!

With pretty mode, compiled code was:

_hamlout.push_text("<p>foo</p>\n#{
_hamlout.format_script(( haml_concat 'foo'
),false,false,false,false,false,true,false);}\n<p>bar</p>\n", 0, false);;::Haml::Util.html_safe(_erbout)

and the backtrace:

/home/k0kubun/src/github.com/haml/haml/lib/haml/buffer.rb:134:in `format_script'
(haml):2:in `block in render'
/home/k0kubun/src/github.com/haml/haml/lib/haml/engine.rb:126:in `eval'
/home/k0kubun/src/github.com/haml/haml/lib/haml/engine.rb:126:in `render'
/home/k0kubun/src/github.com/haml/haml/test/test_helper.rb:65:in `render'
test/helper_test.rb:41:in `render'
test/helper_test.rb:562:in `test_error_return_line'

And with your branch, compiled code is:

_hamlout.buffer << ("<p>foo</p>\n".freeze);; _hamlout.buffer << (
( haml_concat 'foo'
).to_s);; _hamlout.buffer << ("\n<p>bar</p>\n".freeze);;::Haml::Util.html_safe(_erbout)

And the backtrace is:

(haml):3:in `block in render'
/home/k0kubun/src/github.com/haml/haml/lib/haml/engine.rb:126:in `eval'
/home/k0kubun/src/github.com/haml/haml/lib/haml/engine.rb:126:in `render'
/home/k0kubun/src/github.com/haml/haml/test/test_helper.rb:64:in `render'
test/helper_test.rb:41:in `render'
test/helper_test.rb:562:in `test_error_return_line'

Haml::Error is raised by Haml::Helpers::ErrorReturn#to_s and to_s is in 3rd line. In pretty code, format_script in 2nd line did to_s. And this to_s must be in 3rd line considering compilation because if it's in 2nd line = foo # comment like script won't work.

This test is too fragile. How about calling to_s explicitly in the 2nd line?

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Feb 9, 2017

I am not sure about the escaping in a 2 tests in EngineTest - help!

It seems that it's the current behavior.

$ haml -v
Haml 4.1.0.beta.1
$ cat in.haml
%textarea&= "foo\nbar"
$ haml in.haml 
<textarea>foo&#x000A;bar</textarea>
$ haml -t ugly in.haml 
<textarea>foo
bar</textarea>
$ haml -v
Haml 4.0.7
$ cat in.haml
%textarea&= "foo\nbar"
$ haml in.haml 
<textarea>foo&#x000A;bar</textarea>
$ haml -t ugly in.haml 
<textarea>foo
bar</textarea>
@teeparham
Copy link
Member Author

@teeparham teeparham commented Feb 9, 2017

Thanks @k0kubun!

This is ready except for the haml-spec update. I do not have commit permissions to update that repository.

@teeparham
Copy link
Member Author

@teeparham teeparham commented Feb 10, 2017

The git submodule update does not work since the haml-spec commit referenced in 8719e44 is not in the master branch of haml-spec. Tests will pass when that is merged (I will amend the last commit when that happens).

To run the tests locally:

git fetch origin pull/894/head:pr-894
git checkout pr-894
cd test/haml-spec/
git fetch origin pull/13/head:pr-13
git checkout pr-13
cd ../..
bundle exec rake
@amatsuda
Copy link
Member

@amatsuda amatsuda commented Feb 10, 2017

@teeparham teeparham force-pushed the rm-ugly branch from 8719e44 to 1691015 Feb 10, 2017
@teeparham
Copy link
Member Author

@teeparham teeparham commented Feb 10, 2017

@amatsuda It looks like haml/haml-spec#7 doesn't work with Rails 4.0. I think we need haml/haml-spec#10 instead. Otherwise, tests are passing.

@teeparham
Copy link
Member Author

@teeparham teeparham commented Feb 10, 2017

We could remove rails 4.0 from the test suite, since it's unsupported.

@amatsuda
Copy link
Member

@amatsuda amatsuda commented Feb 10, 2017

@teeparham Ugh, sorry. Fixed haml-spec.

@teeparham teeparham force-pushed the rm-ugly branch from 1691015 to 70af4d7 Feb 10, 2017
@teeparham
Copy link
Member Author

@teeparham teeparham commented Feb 10, 2017

@amatsuda Thank you. Tests are green!

teeparham added 5 commits Feb 2, 2017
* Remove unused indent argument from push_merged_text
* Remove ugly argument from #format_script

(Indentation to be fixed in the next commit. This makes the commit
easier to understand.)
koic added a commit to koic/rails_admin that referenced this pull request Feb 27, 2017
k0kubun added a commit to k0kubun/hamlit that referenced this pull request May 3, 2017
Now Haml doesn't have pretty mode haml/haml#894.
joshukraine added a commit to joshukraine/middleman-gulp that referenced this pull request May 5, 2017
tekniklr pushed a commit to tekniklr/tekniklr.com that referenced this pull request May 13, 2017
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 16, 2017
This option got dropped from haml. See haml/haml#894
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 20, 2017
This option got dropped from haml. See haml/haml#894
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 20, 2017
This option got dropped from haml. See haml/haml#894
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 20, 2017
This option got dropped from haml. See haml/haml#894
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 21, 2017
This option got dropped from haml. See haml/haml#894
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 21, 2017
This option got dropped from haml. See haml/haml#894
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 21, 2017
This option got dropped from haml. See haml/haml#894
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 21, 2017
This option got dropped from haml. See haml/haml#894
bgeuken added a commit to bgeuken/open-build-service that referenced this pull request Jun 21, 2017
This option got dropped from haml. See haml/haml#894
dblock added a commit to dblock/doppler that referenced this pull request Oct 11, 2017
jeremyz added a commit to jeremyz/zorglub that referenced this pull request Jan 15, 2018
niosHD added a commit to niosHD/gitstats-rb that referenced this pull request Oct 17, 2019
The :ugly option was removed from Haml in the 5.0.0 release [1,2].

[1] https://github.com/haml/haml/blob/master/CHANGELOG.md#500
[2] haml/haml#894
issyl0 added a commit to issyl0/diesel.rs-website that referenced this pull request May 24, 2020
- It was removed in HAML haml/haml#894.
issyl0 added a commit to issyl0/diesel.rs-website that referenced this pull request May 24, 2020
- It was removed in HAML haml/haml#894.
issyl0 added a commit to issyl0/diesel.rs-website that referenced this pull request May 28, 2020
- It was removed in HAML haml/haml#894.
issyl0 added a commit to issyl0/diesel.rs-website that referenced this pull request May 28, 2020
- It was removed in HAML haml/haml#894.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.