Clarify config handling & template lookup #130
Conversation
Raise another error if paths are missing from config
lib/dry/view.rb
Outdated
| # @api private | ||
| def ensure_config | ||
| raise UndefinedPathsError.new unless Array(config.paths).any? | ||
| raise UndefinedTemplateError.new unless config.template |
There was a problem hiding this comment.
@solnic @flash-gordon Style question for you. Which would you consider better:
- Different error classes for each bit of config that is critical to rendering, like we have here, or
- A single
InvalidConfigerror that we raise with different messages based on which piece of config is invalid?
There was a problem hiding this comment.
@jodosha Actually, this might be good to ask you, too. Would you rather have a single class for config-related errors, or an individual class for each specific config key that could be invalid?
There was a problem hiding this comment.
@timriley No strong preference here. As a general rule that I follow, it's worth to have specific error objects, if you need to enrich them with debug information.
For instance the error UndefinedPathsError can be useful if raised with configured paths:
raise UndefinedPathsError.new(config.paths) unless Array(config.paths).any?So devs will see an informative error message:
Dry::View::UndefinedPathsError: cannot find paths: `path/to/missing/directory'
There was a problem hiding this comment.
@jodosha Thanks for the feedback :) In this case I decided to unify the errors and pass through the missing configuration key as the extra data:
raise UndefinedConfigError.new(:paths) unless Array(config.paths).any?
raise UndefinedConfigError.new(:template) unless config.template
timriley
changed the title
|
This is ready to merge, just leaving this open so I can get @solnic / @flash-gordon's feedback on the error classes thing. |
Centralize validation of view class-level config, and along with the error that we raise for a not-configured
template, raise another error if thepathsare not configured.Clarify template lookup:
shared/subdirs when rendering partials.Refactor template lookup code so it's easier to follow. Add extra values to the result cache key to avoid incorrect cache hits.
Move TemplateNotFoundError into main
Dry::Viewnamespace, since it's likely something that will be be encountered by users, and they don't need to see the nesting withinDry::View::Renderer.