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

Add support for linting ECR files #536

Merged
merged 4 commits into from
Jan 15, 2025

Conversation

nobodywasishere
Copy link
Contributor

Requires Crystal >= 1.15.0. Lints ECR files by default. Closes #509

Requires Crystal >= 1.15.0
src/ameba/source.cr Outdated Show resolved Hide resolved
@Sija Sija requested a review from veelenga January 12, 2025 13:03
@Sija
Copy link
Member

Sija commented Jan 12, 2025

Specs would be good to have too 🙏

@Sija Sija added this to the 1.7.0 milestone Jan 12, 2025
src/ameba/glob_utils.cr Outdated Show resolved Hide resolved
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
@veelenga
Copy link
Member

@nobodywasishere I may have missed some conversation, but does this conflict with Lint/UselessAssign?

@nobodywasishere
Copy link
Contributor Author

nobodywasishere commented Jan 12, 2025

I don't know if there's been a conversation about that, but I do see how this could conflict with Lint/UselessAssign. As ECR files are expanded as Crystal code in-place, this can lead to stuff like this:

def render_page
  my_var = 1
  # ^^^^ error: Useless assignment to `my_var`
  render("path/to/page.ecr")
end
<%= my_var %>

This is already an issue though without this PR. Is there something specific you had in mind?

Another one specific to ECR though is something like:

<%
  my_var ||= 2
  # ^^^^ Lint/Syntax: Cannot use `my_var` in assignment to `my_var`
%>
<%= my_var %>

Which means templates just need to be written in a way where they're parsable outside the context of where they're rendered.


One cool thing I found while testing is that Lint/UnusedLiteral works:

<% 
  "hello world" 
  # ^^^^^^^^^^^ error: unused literal
%>

So it will give a warning if you accidentally leave off the =.

@nobodywasishere
Copy link
Contributor Author

I've gone through all the rules and all the current auto corrections should be fine. Anything that changes stuff over the <% / %> boundary though would be a problem. For example, if there was a rule that commented out the contents of an if for some reason:

Original ECR:

<% if true %>
  <%= "hello" %>
<% end %>

Parsed ECR (without loc pragmas):

if true
  __str__ << "\n  "
  ("hello").to_s(__str__)
  __str__ << "\n"
end

"Corrected" source:

if true
  # __str__ << "\n  "
  # ("hello").to_s(__str__)
  # __str__ << "\n"
end

Could completely break or mangle the original ECR. This shouldn't be a problem with the current rules though.

@Sija
Copy link
Member

Sija commented Jan 13, 2025

By specs I meant testing the ECR support, not adding tests to the rule specs.

src/ameba/source.cr Outdated Show resolved Hide resolved
spec/ameba/source_spec.cr Outdated Show resolved Hide resolved
@nobodywasishere
Copy link
Contributor Author

I thought having some tests that ensure functionality within an ECR file would be useful, but I can remove them if desired.

src/ameba.cr Show resolved Hide resolved
Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

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

lgtm

src/ameba/config.cr Outdated Show resolved Hide resolved
@Sija Sija merged commit 371414e into crystal-ameba:master Jan 15, 2025
4 checks passed
Sija added a commit that referenced this pull request Jan 15, 2025
@nobodywasishere nobodywasishere deleted the nobody/ecr-lint branch January 15, 2025 05:23
@nobodywasishere nobodywasishere restored the nobody/ecr-lint branch January 15, 2025 05:23
Sija added a commit that referenced this pull request Jan 15, 2025
@Sija
Copy link
Member

Sija commented Jan 15, 2025

@nobodywasishere
Copy link
Contributor Author

I've no idea what caused that that wouldn't also work locally... I can try moving the location of the require "ecr/processor" to see if that'd work, but other than that unless it's building using an older version of Crystal (<1.15), I've no idea.

@nobodywasishere
Copy link
Contributor Author

Yeah alpine is still using Crystal 1.14 https://pkgs.alpinelinux.org/package/edge/community/x86_64/crystal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for linting ECR files
3 participants