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

Remove LayoutNotFoundError and raise TemplateNotFoundError in all cases #247

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

timriley
Copy link
Member

@timriley timriley commented Nov 4, 2023

Remove the specialized LayoutNotFoundError, because this was raising incorrect error if there were missing partials being rendered inside the layout.

It is better that we raise a genuine, non-misleading error in these cases. The LayoutNotFoundError only provided marginal utility anyway, and the inclusion of the “layouts/“ prefix for a TemplateNotFoundError raised for a missing layout will still make it clear where the template was being looked for.

Fixes #246.

Remove the specialized LayoutNotFoundError, because this was raising incorrect error if there were _missing partials_ being rendered inside the layout.

It is better that we raise a genuine, non-misleading error in these cases. The LayoutNotFoundError only provided marginal utility anyway, and the inclusion of the “layouts/“ prefix for a TemplateNotFoundError raised for a missing layout will still make it clear where the template was being looked for.
@timriley
Copy link
Member Author

timriley commented Nov 4, 2023

The LayoutNotFoundError was introduced in #191 and has never been formally released, so it is safe to remove here.

@timriley timriley requested a review from jodosha November 4, 2023 05:37
@timriley timriley self-assigned this Nov 4, 2023
@timriley timriley added this to the v2.1.0 milestone Nov 4, 2023
@timriley timriley changed the title Raise TemplateNotFoundError in all cases Remove LayoutNotFoundError and raise TemplateNotFoundError in all cases Nov 4, 2023
Copy link
Member

@jodosha jodosha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timriley I agree with the change. Please clarify my inline question.

config.template = "missing_template"
}.new

expect { view.() }.to raise_error Hanami::View::TemplateNotFoundError, /Template `missing_template' for format `html' could not be found/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timriley In case of Hanami app, will the real error message appear with the relative path of the template?

Example:

Template `app/templates/layouts/foo.html.erb' for format `html' could not be found

As a dev, I can just copy and paste the path and create the missing template.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodosha Here's the exact error:

     Hanami::View::TemplateNotFoundError:
       Template `layouts/app' for format `html' could not be found in paths:

        - /Users/tim/Source/scratch/bookshelf/app/templates

So yes, you could join layouts/app onto /Users/tim/Source/scratch/bookshelf/app/templates and there you have the correct path to use (minus the file extension).

We can't put an exact file extension on this because we do support fuzzy loading e.g. from any *.html.* template files.

I think we could probably finesse this error message to make it even nicer. For example, if there's only one path configured, then we could make the error a lot closer to your example.

However, I'd like to leave that as an exercise for a future enhancement rather than block on this now. This would be a great first issue for a potential Hanami contributor, for example 😄

@timriley timriley merged commit 1129830 into main Nov 7, 2023
@jodosha jodosha deleted the remove-layoutnotfounderror branch November 7, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Missing templates result in (incorrect) missing layout error
2 participants