zendesk/curly

Discrepancy in usage of presenter_for_path and presenter_for_name

jeremyf opened this issue · 5 comments

Consider the following, when we call a presenter_for_path we allow for nil to be returned. When we call presenter_for_name, nil is not an option instead we raise Curly::PresenterNameError. The following line seems redundant, given that we can change presenter_for_path to bubble up with a PresenterNameError (or PresenterNotFound in both cases).

I don't know if these methods are considered part of the public API, but it is something to consider.

presenter_class = Curly::Presenter.presenter_for_path(path)

raise Curly::PresenterNotFound.new(path) if presenter_class.nil?

https://github.com/zendesk/curly/blob/master/lib/curly/template_handler.rb#L48-L50

def presenter_for_path(path)
  begin
    presenter_for_name(path.camelize, [])
  rescue Curly::PresenterNameError
    nil
  end
end

https://github.com/zendesk/curly/blob/master/lib/curly/presenter.rb#L158-L166

dasch commented

It's a public API, so it may break clients that use those methods :-/

So, any pull request would necessitate a major version bump? A deprecation strategy?

dasch commented

Yeah, stuff like that. It's typically not worth it.

@dasch I yield to you regarding what you want to do? Close the issue or keep it open and say "help wanted"

dasch commented

I'd rather close this – it doesn't seem like a huge problem, and API stability is more important.