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 Style/HeredocIndent #554

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nobodywasishere
Copy link
Contributor

Enforces that heredoc bodies are indented consistently.

@Sija Sija added the rule label Jan 29, 2025
@Sija Sija added this to the 1.7.0 milestone Jan 29, 2025

it "passes if heredoc body indented one level" do
expect_no_issues subject, <<-CRYSTAL
call <<-HEREDOC
Copy link
Member

Choose a reason for hiding this comment

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

We're testing just the heredoc strings, no need for additional constructs.

Suggested change
call <<-HEREDOC
<<-HEREDOC

ditto elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The call is needed to make sure the error # ^^^^ is within the heredoc body indentation-wise

Copy link
Member

Choose a reason for hiding this comment

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

That's handled by the ^{} marker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I mean is if the body is indented, the marker can't point to the beginning of the heredoc. This will give a parsing error:

<<-HEREDOC
# ^^^^^^^^ error: Heredoc indent ....
  HEREDOC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we're testing the indent relative to the indent of w/e is on the line where the heredoc start is, not to the indent of the heredoc start itself

src/ameba/rule/style/heredoc_indent.cr Outdated Show resolved Hide resolved
src/ameba/rule/style/heredoc_indent.cr Outdated Show resolved Hide resolved
src/ameba/rule/style/heredoc_indent.cr Outdated Show resolved Hide resolved
src/ameba/rule/style/heredoc_indent.cr Outdated Show resolved Hide resolved
src/ameba/rule/style/heredoc_indent.cr Outdated Show resolved Hide resolved
src/ameba/rule/style/heredoc_indent.cr Outdated Show resolved Hide resolved
src/ameba/rule/style/heredoc_indent.cr Outdated Show resolved Hide resolved
Comment on lines +50 to +54
correct_indent = line_indent(source, start_location) + indent_by

unless node.heredoc_indent == correct_indent
issue_for node, MSG % correct_indent
end
Copy link
Member

Choose a reason for hiding this comment

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

Indentation should measured relatively to the Heredoc string.

Suggested change
correct_indent = line_indent(source, start_location) + indent_by
unless node.heredoc_indent == correct_indent
issue_for node, MSG % correct_indent
end
unless node.heredoc_indent == indent_by
issue_for node, MSG % indent_by
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow. heredoc_indent is the absolute indent of the heredoc ending identifier (not the relative indent), which is used to strip the extra whitespace from the beginning of the heredoc body. We therefore need to compare it against the desired absolute indent. With that, the heredoc body would always have to be indented by 2 from the beginning of the line.

Copy link
Member

Choose a reason for hiding this comment

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

Desired indent should always be relative, not absolute.

Copy link
Contributor Author

@nobodywasishere nobodywasishere Jan 29, 2025

Choose a reason for hiding this comment

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

With the change you're suggesting, a heredoc would have to be indented like this to pass:

class MyClass
  def my_method
    var = <<-HEREDOC
  This is the value for `var`
  HEREDOC
  end
end

With the existing code, it has to be formatted like this:

class MyClass
  def my_method
    var = <<-HEREDOC
      This is the value for `var`
      HEREDOC
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

StringInterpolation#heredoc_indent is counted from the start of the heredoc line, not start of the line, thus I think you're mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  call <<-HEREDOC
    body of heredoc
    HEREDOC
^^^^
# This is the heredoc_indent

With the proposed change, the heredoc body fails unless it's indented with two spaces, regardless of the indentation of the line the heredoc is used on:

image

Copy link
Member

Choose a reason for hiding this comment

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

That's exactly the point. Line indentation shouldn't matter here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely disagree. That way of formatting heredocs does not match what is used in basically all of the codebases I work with, including this one. Having the body indented two lines while the rest of the code could be 6/8/10 lines indented does not look good.

src/ameba/rule/style/heredoc_indent.cr Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants