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

Render using absolute path #146

Closed
wants to merge 1 commit into from
Closed

Render using absolute path #146

wants to merge 1 commit into from

Conversation

faustinoaq
Copy link
Contributor

Description of the Change

This PR add absolute path to macro render_template.

Alternate Designs

The absolute path is used if path is equal to "src/view"

Why Should This Be In Core?

Currently in my amber projects I have to rewrite the macro render_template adding support for absolute path, because vscode-crystal-lang extension can't find relative paths when analyze code.

My application_controller.cr

class ApplicationController < Amber::Controller::Base
  LAYOUT = "application.ecr"

  macro render_template(filename, path = "src/views")
    {% if filename.id.split("/").size > 2 %}
      Kilt.render("{{filename.id}}")
    {% else %}
      {% if path == "src/views" %}
        Kilt.render("{{__FILE__.split('/')[0..-4].join('/').id}}/#{{{path}}}/{{filename.id}}")
      {% else %}
        Kilt.render("#{{{path}}}/{{filename.id}}")
      {% end %}
    {% end %}
  end
end

Some screenshots:

Show macro errors related to relative path of views:

screenshot_20170721_205722

Errors fixed using absolute path:

screenshot_20170721_205917

Benefits

This change add absolute path only if path is equal to "src/view" so, if path change the render process uses the path given.

Possible Drawbacks

Absolute path is calculated using __FILE__ only when path is default to views folder, if path changes then absolute path isn't used.

Applicable Issues

Editor extensions like Scry can't find relative paths used by macros, so this PR help to users to avoid unnecessary macro errors.

Copy link
Member

@elorest elorest left a comment

Choose a reason for hiding this comment

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

Not sure about this implementation.

@@ -6,7 +6,11 @@ module Amber::Controller
{% if filename.id.split("/").size > 2 %}
Kilt.render("{{filename.id}}")
{% else %}
Kilt.render("#{{{path}}}/{{filename.id}}")
{% if path == "src/views" %}
Kilt.render("{{__FILE__.split('/')[0..-4].join('/').id}}/#{{{path}}}/{{filename.id}}")
Copy link
Member

@elorest elorest Jul 22, 2017

Choose a reason for hiding this comment

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

If absolute path is important then we should probably add so that all renders take it into account. Using it only when path = "src/views" only fixes this partially assuming it's something that actually needs fixed.

Seems a little funny using absolute paths for views in a web project though. Could you perhaps just fix your editor?

Copy link
Member

Choose a reason for hiding this comment

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

__FILE__.split('/')[0..-4] assumes that __FILE__ is always going to be at the same level. If someone were to render a view from presenter, or even another ecr file it will fail.

@faustinoaq
Copy link
Contributor Author

faustinoaq commented Jul 22, 2017 via email

@faustinoaq
Copy link
Contributor Author

This error also is happening with Kemal and other Crystal programs, so I opened a issue in vscode-crystal-lang instead 👉 https://github.com/faustinoaq/vscode-crystal-lang/issues/5

@faustinoaq faustinoaq closed this Jul 24, 2017
@faustinoaq faustinoaq deleted the patch-1 branch July 24, 2017 14:59
@faustinoaq
Copy link
Contributor Author

Now, My editor extension is fixed https://github.com/faustinoaq/vscode-crystal-lang/pull/10 😄

@faustinoaq faustinoaq added this to Done in Framework 2018 May 5, 2018
@faustinoaq faustinoaq removed this from Done in Framework 2018 Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants