Indentation checker does not check for single space indentation

[PatrickMcSweeny]

I created a test locally to see if the indentation checker would approve a solution which used single space indentation, and it did approve the solution. This is because the indentation checker only checks to see if the difference in spaces between a line and the previous line is greater than two. A better solution would be to see if the difference in spaces is equal to zero or two. We could probably get even fancier using some of the functionality in the Rubocop AST library.

I would like to work on a PR to address this as well as the refactoring that is suggested here.

[github-actions]

Hello. Thanks for opening an issue on Exercism 🙂

At Exercism we use our Community Forum, not GitHub issues, as the primary place for discussion. That allows maintainers and contributors from across Exercism's ecosystem to discuss your problems/ideas/suggestions without them having to subscribe to hundreds of repositories.

This issue will be automatically closed. Please use this link to copy your GitHub Issue into a new topic on the forum, where we look forward to chatting with you!

If you're interested in learning more about this auto-responder, please read this blog post.

[kotp]

The single space should not be a failure and analyzer approval would be warranted. The two space indentation in Ruby is convention, but not necessary.

More significant would be a check that indentation is consistent, but this is also tricky, because when that deviates, it may be there to subtly communicate something about that area of the code.

I would not worry it too much.

This highlights why discussions should happen before these things are created, to avoid notifications here on work that is not really wanted. Consider using github issues as a a "TODO" registry, rather than a "MIGHT DO" registry.

That said, since we are here, what is your refactoring idea for the second part?

[kotp]

I said "is not really wanted" but I think we will come to the agreement that it is wanted. I want to be clear that I am not discouraging the work suggested here. Only that we could discuss it to be clear what the goal is.

That said, I think if it is detected that 1 space indentation is here, that we would report that it is a well accepted convention in Ruby code to use two spaces for indentation. Not tab or one space, but two spaces. It it would be related to write an analyzer that detects trailing white space as well. Even lack of EOL on what should be the last line.

So we have plenty of areas that could be pointed out in the analyzer.

[PatrickMcSweeny]

That said, since we are here, what is your refactoring idea for the second part?

I mistakenly linked to the wrong line, but there is a comment in the indentation checker saying that the blank lines could be stripped and then each_cons could be used to check two lines at a time. This would make for a more readable solution, and it would probably be better for performance as well.

[kotp]

I mistakenly linked to the wrong line

Did you edit the post to correct the link for ease of navigation, discussion? I did not notice it. I will try to figure out and then search for what you describe though. I believe you mean solution_representation.rb:+9. It is also the only other comment in the file that was linked to.

This would make for a more readable solution, and it would probably be better for performance as well.

What do you have?

[PatrickMcSweeny]

PatrickMcSweeny@0ab6329

[kotp]

PatrickMcSweeny@0ab6329

diff --git a/lib/analyzers/solution_representation.rb b/lib/analyzers/solution_representation.rb
index efef5c7..c306eff 100644
--- a/lib/analyzers/solution_representation.rb
+++ b/lib/analyzers/solution_representation.rb
@@ -6,28 +6,13 @@ class SolutionRepresentation
     @code_to_analyze = code_to_analyze
   end
 
-  # REFACTOR: This could be refactored to strip blank
-  # lines and then use each_cons(2).
   def indentation_is_sensible?
-    previous_line = nil
-    code_to_analyze.lines.each do |line|
-      # If the previous line or this line is
-      # just a whitespace line, don't consider it
-      # when checking for indentation
-      unless previous_line.nil? ||
-             previous_line =~ /^\s*\n*$/ ||
-             line =~ /^\s*\n*$/
+    code_to_analyze.lines.reject { |line| line =~ /^\s*\n*$/ }.each_cons(2).all? do |lines|
+      line_1_space = lines.first[/^ */].size
+      line_2_space = lines.last[/^ */].size
 
-        previous_line_lspace = previous_line[/^ */].size
-        line_lspace = line[/^ */].size
-
-        return false if (previous_line_lspace - line_lspace).abs > 2
-      end
-
-      previous_line = line
+      (line_2_space - line_1_space).abs <= 2
     end
-
-    true
   end
 
   def has_target_module?

I see.

Which results in having:

  def indentation_is_sensible?
    code_to_analyze.lines.reject { |line| line =~ /^\s*\n*$/ }.each_cons(2).all? do |lines|
      line_1_space = lines.first[/^ */].size
      line_2_space = lines.last[/^ */].size

      (line_2_space - line_1_space).abs <= 2
    end
  end

Rather than

  def indentation_is_sensible?
    previous_line = nil
    code_to_analyze.lines.each do |line|
      # If the previous line or this line is
      # just a whitespace line, don't consider it
      # when checking for indentation
      unless previous_line.nil? ||
             previous_line =~ /^\s*\n*$/ ||
             line =~ /^\s*\n*$/

        previous_line_lspace = previous_line[/^ */].size
        line_lspace = line[/^ */].size

        return false if (previous_line_lspace - line_lspace).abs > 2
      end

      previous_line = line
    end

    true
  end

Consider if we are only checking line 1 and 2. The original has references that are relative, which makes sense to me, rather than specific line numbers, which does not seem to reflect what is happening here. (Naming is hard, I am not sure what to name them, but partly because it is pulling my mind to line 1 and line 2, as it reads, rather than how it is probably intended.

---

Do we also have a related test to avoid regression of behavior?

[PatrickMcSweeny]

There are a series of indentation tests here: https://github.com/exercism/ruby-analyzer/blob/main/test/exercises/two_fer_test.rb#L321

[kotp]

There are a series of indentation tests here: exericsm/ruby-analyzer/two_fer_test.rb#L321

Yes, but I was asking for the Leap Year exercise. This is something that you would likely put together. You can use the information you found to model that, though.

[PatrickMcSweeny]

The leap year analyzer does not currently check for indentation because it does not auto-approve submissions.

[kotp]

I see what you are saying. I was asking about tests, if they needed to be changed. I just ran the tests against your changes, and they still pass, so from what I can see, nothing broke, but I am doubting that the title of this issue is addressed via testing, since currently no single indentation is checked, and this change is meant to check for single space indentation. Should we add a test to help prevent regressions?

I like the code change. I think it is easier to read, addresses the comment concern, but would ask that names are considered for making things clear when we come back to read the code later. current_line and prior_line might be names that I would use rather than line_1, line_2 as part of the name, or whatever helps to make that code's intention clear.

To reiterate, if the current code does not check for a single space indentation, we likely want to make a test that ensures we do not have a regression, right? A change without a test means that it would be easy to not see that some maintenance change reverts it back to not checking for the single space indentation.

[PatrickMcSweeny]

The commit I posted was a refactoring to improve readability and performance. It was not meant to check for single-space indentation. If you are interested in checking to make sure that indentation is done with two spaces, I could add that to my proposed change.

[PatrickMcSweeny]

This commit shows how I would check for two-space indentation:

PatrickMcSweeny@bc0bbd3

[kotp]

This commit shows how I would check for two-space indentation:

PatrickMcSweeny@bc0bbd3

For ease of reading:

diff --git c/lib/analyzers/solution_representation.rb w/lib/analyzers/solution_representation.rb
index efef5c7..2e9adb5 100644
--- c/lib/analyzers/solution_representation.rb
+++ w/lib/analyzers/solution_representation.rb
@@ -6,28 +6,13 @@ class SolutionRepresentation
     @code_to_analyze = code_to_analyze
   end
 
-  # REFACTOR: This could be refactored to strip blank
-  # lines and then use each_cons(2).
   def indentation_is_sensible?
-    previous_line = nil
-    code_to_analyze.lines.each do |line|
-      # If the previous line or this line is
-      # just a whitespace line, don't consider it
-      # when checking for indentation
-      unless previous_line.nil? ||
-             previous_line =~ /^\s*\n*$/ ||
-             line =~ /^\s*\n*$/
+    code_to_analyze.lines.reject { |line| line =~ /^\s*\n*$/ }.each_cons(2).all? do |lines|
+      line_1_space = lines.first[/^ */].size
+      line_2_space = lines.last[/^ */].size
 
-        previous_line_lspace = previous_line[/^ */].size
-        line_lspace = line[/^ */].size
-
-        return false if (previous_line_lspace - line_lspace).abs > 2
-      end
-
-      previous_line = line
+      [0, 2].include? (line_2_space - line_1_space).abs
     end
-
-    true
   end
 
   def has_target_module?
diff --git c/test/exercises/two_fer_test.rb w/test/exercises/two_fer_test.rb
index 3b321f8..652c827 100644
--- c/test/exercises/two_fer_test.rb
+++ w/test/exercises/two_fer_test.rb
@@ -336,7 +336,13 @@ class TwoFerTest < Minitest::Test
         "One for %s, one for me." % name
      end
     end
-  '].each.with_index do |source, idx|
+  ', '
+    class TwoFer
+     def self.two_fer(name="you")
+     "One for %s, one for me." % name
+     end
+    end'
+  ].each.with_index do |source, idx|
     define_method "test_incorrect_indentation_#{idx}" do
       # skip
       results = TwoFer::Analyze.(source)