Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions spec/ameba/rule/layout/trailing_whitespace_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,28 @@ module Ameba::Rule::Layout
expect_no_issues subject, "no-whitespace"
end

it "passes a line ends with trailing CRLF sequence" do
expect_no_issues subject, "no-whitespace\r\n"
end

it "fails if a line ends with trailing \\r character" do
source = expect_issue subject, <<-TEXT
carriage return at the end\r
# ^ error: Trailing whitespace detected
TEXT

expect_correction source, "carriage return at the end"
end

it "fails if there is a line with trailing tab" do
source = expect_issue subject, <<-TEXT
tab at the end\t
# ^ error: Trailing whitespace detected
TEXT

expect_correction source, "tab at the end"
end

it "fails if there is a line with trailing whitespace" do
source = expect_issue subject,
"whitespace at the end \n" \
Expand Down
2 changes: 1 addition & 1 deletion src/ameba/source.cr
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ module Ameba
# source = Ameba::Source.new "a = 1\nb = 2", path
# source.lines # => ["a = 1", "b = 2"]
# ```
getter lines : Array(String) { code.split('\n') }
getter lines : Array(String) { code.split(/\r?\n/) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason not to use String#lines? Should be much more efficient.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, String#lines eats the final newline:

str = <<-TEXT
  foo  
  bar\t
  
  TEXT
 
str.lines          # => ["foo  ", "bar\t", ""]
str.split(/\r?\n/) # => ["foo  ", "bar\t", "", ""]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is that really relevant, though?
I believe this should only matter for Lint/TrailingBlankLines which could do a simpler, string-based test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

         (\n) lines   5.58k (179.32µs) (± 5.45%)  527kB/op        fastest
   (\n) regex split   2.12k (471.77µs) (± 4.67%)  729kB/op   2.63× slower
  (\n) gsub + split   2.28k (438.07µs) (± 3.47%)  527kB/op   2.44× slower
       (\r\n) lines   5.47k (182.86µs) (± 2.04%)  527kB/op   1.02× slower
 (\r\n) regex split   2.05k (487.75µs) (± 0.91%)  729kB/op   2.72× slower
(\r\n) gsub + split   1.74k (573.81µs) (± 1.94%)  721kB/op   3.20× slower

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Source#lines is expected to return accurate representation of the underlying data, without any modifications, so I'd prefer to stick to correctness over efficiency here.

On a related note, why the String#lines is eating the final newline? It seems to me that's either a bug, or an undocumented behavior at least.

Copy link
Copy Markdown
Contributor

@straight-shoota straight-shoota Jun 11, 2025

Choose a reason for hiding this comment

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

I think both interpretations are valid. There's no clear answer whether \n is one or two lines. We have to live with the ambiguity.
The reasoning for one is: The line feed character \n marks the end of a line. That does not necessarily imply the start of a new line. At the end of the file/string there's no content afterwards. So it's reasonable to not counting a trailing newline as an additional empty line.

The documentation is certainly sparse on this, though. String#lines has no docs whatsoever. String#each_line says it "splits the string after each newline", which matches the observed behaviour.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree. Having a keep_trailing_newline = false named parameter added to String#lines would help to achieve both behaviors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR to improve docs: crystal-lang/crystal#15894


# Returns AST nodes constructed by `Crystal::Parser`.
#
Expand Down
Loading