premailer / css_parser

Ruby CSS Parser
Other
279 stars 110 forks source link

Whats with all the Prima Donna methods? #110

Closed DannyBen closed 4 years ago

DannyBen commented 4 years ago

What is the reason behind your public methods mostly ending with an exclamation mark?

This practice is usually reserved for dangerous methods, but only assuming they have a safe alternative (reference, another reference).

It feels very unnatural to write load_string! instead of just load_string...

grosser commented 4 years ago

I think they are ! because they modify the parser ... which seems wrong if you can confirm the don't modify the passed in object I'd merge a PR to alias them to non-bang/update the Readme

DannyBen commented 4 years ago

Well, I guess they are modifying it, but I can't confirm it, since I am unfamiliar with the code.

I don't think aliasing is worth the trouble, instead, perhaps just consider this for the next breaking change release?

By the way - even if a method changes the object, the common guidelines dictate that it should not end with a bang. It can only end with a bang if it has a non-bang alternative.

Also - and this is probably an opinionated statement - perhaps this whole problem can be avoided, with a pattern similar to the next one:

class Parser
  def self.load_uri(uri)
    new(get_string_from_uri uri)
  end

  def self.load_file(path)
    new(File.read path)
  end

  def initialize(string)
    @string = string
  end

  def to_h
    # ...
  end
end
grosser commented 4 years ago

yeah not worth the trouble, doubt anyone uses it like that ... seems good enough as it is then and since it modifies the parser the ! is alright 🤷