threedaymonk / htmlbeautifier

A normaliser/beautifier for HTML that also understands embedded Ruby. Ideal for tidying up Rails templates.
MIT License
325 stars 59 forks source link

Add a convenient method HtmlBeautifier.beautify #10

Closed jirutka closed 9 years ago

jirutka commented 9 years ago

The current API is IMHO quite weird and inconvenient for the most common use case – just take HTML in String and return it in a prettier shape, also as String. This should be one-line, not three lines in a procedural feel.

output = []
Beautifier.new(output).scan(input)
output.join

I’ve added a shorthand method beautify to HtmlBeautifier module that does exactly that — and also call to_html method on the given input, when it responds to it (e.g. Nokogiri node).

threedaymonk commented 9 years ago

The API isn't quite as weird as your example: you can use anything that accepts <<, which includes a string. However, I agree that it's a bit strange.

My intention was for it to write directly to standard output when being used as a command line tool, but I don't think this is really useful in any real-life scenario.

Rather than wrap the interface weirdness, I'd prefer to make it less weird, which I've started doing in the improve-api branch: fb08d73

jirutka commented 9 years ago

Rather than wrap the interface weirdness, I'd prefer to make it less weird.

Agree, I just wanted to preserve backward compatibility.

I've started doing in the improve-api branch: fb08d73 After: output = HtmlBeautifier::Beautifier.new.beautify(html)

Isn’t the Beautifier class totally useless here? I’d prefer:

output = HtmlBeautifier.beautify(html)
output = HtmlBeautifier.beautify(html, tab_stops: 4)

What do you think?

(I’ve updated this PR)

threedaymonk commented 9 years ago

Yeah, I like this.

I'd like to retain compatibility with Ruby 1.9 for now, so I'll make a few changes to avoid the keyword arguments and merge it.

jirutka commented 9 years ago

The keyword argument is not necessary here, but it’s IMO better for future extensibility (adding another options). You can also use just Hash instead.

threedaymonk commented 9 years ago

Yep, I went with an options hash as that's compatible and extensible.

threedaymonk commented 9 years ago

Merged along with additional commits in 78e747f.

jirutka commented 9 years ago

Great! :smiley_cat:

Please remember that this is not backward compatible change of the public API, so you should increment major version number (so you’ll get 1.0.0 :+1:).

threedaymonk commented 9 years ago

you should increment major version number (so you’ll get 1.0.0 :+1:).

That's the plan. It'll be the first release with a public API, for that matter!

I want to reorganise the tests a bit (to follow modern conventions) first, so I expect to release tomorrow or later this week.

jirutka commented 9 years ago

I would recommend to use rspec, Minitest::Spec, or at least define a very simple DSL for Minitest::Unit to get_rid_of_these_odd_test_names.

(As for me, you don’t have to rush with a new release.)

threedaymonk commented 9 years ago

Released 1.0.0.

jirutka commented 9 years ago

Great, tests looks much better now! :smile_cat: