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

[Fix #9373]Fix indentation style tabs infinite loop #9377

Conversation

joeyparis
Copy link

@joeyparis joeyparis commented Jan 14, 2021

This fixes the bug described in #9373.

However, the testing for this cop breaks as it designed to replace every 2 spaces, with 1 tab but the fix replaces every 1 space with 1 tab. I would say this is a problem with the fix, but replacing 2 spaces only works in the vacuum of the test and leads to an infinite loop when used along with other cops.

When combined with Layout/IndentationWidth replacing every 1 space with a tab still leads to proper indentation by the time all the cops are done. But I'm assuming this is not ideal behavior for one to have to rely on another like that.

I'm not sure what the proper steps are to finalize this. Do I update the tests to just replace every 1 space with 1 tab? Or should we dive deeper into what else is contributing to the infinite loop?


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote [good commit messages][1].
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 15, 2021

@jonas054 Please, take a look here when you can.

@jonas054
Copy link
Collaborator

jonas054 commented Mar 3, 2021

There are two distinct ways of using tab characters for indentation.

  1. Use tabs with a width of 4 or 8 to save characters in a source code file. Tabs can be followed by spaces to reach the desired indentation level. There's no support in RuboCop for this way. For example,
class MyClass
  def my_method
    @list.each do
      if x
< tab  >unless y
< tab  >  puts 'Yes'
< tab  >end
      end
    end
end
  1. Use tab for indentation, no spaces, and necessarily one tab for one level of indentation. This is possible with RuboCop.
class MyClass
<>def my_method
<><>@list.each do
<><><>if x
<><><><>unless y
<><><><><>puts 'Yes'
<><><><>end
<><><>end
<><>end
end

Using more than one tab per indentation level makes no sense. I think that adding validation so that Layout/IndentationWidth must be set to 1 if EnforcedStyle is tabsin Layout/IndentationStyle would solve the problem.

We should also change the comment # Number of spaces for each indentation level in default.yml to # Number of spaces or tabs for each indentation level.

Copy link
Collaborator

@jonas054 jonas054 left a comment

Choose a reason for hiding this comment

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

I think validating configuration is the way to go, rather than changing the autocorrect behavior. See comment.

@PikachuEXE
Copy link

Seems incompatible with cop Layout/DotPosition when EnforcedStyle: trailing

@jonas054
Copy link
Collaborator

jonas054 commented Jul 5, 2021

@PikachuEXE I'm not so sure that trailing dot position as any different than leading. The alignment with the first line for the chained method calls is just not possible when tabs are used for indentation.

For example, default configuration with spaces gives:

unless y
       .z
  puts 'Yes'
end

while the configuration

Layout/IndentationWidth:
  Width: 1

Layout/IndentationStyle:
  EnforcedStyle: tabs

results in:

unless y
<><><><><><><>.z
<>puts 'Yes'
end

and with the addition of

Layout/DotPosition:
  EnforcedStyle: trailing

we get

unless y.
<><><><><><><>z
<>puts 'Yes'
end

The problem remains the same. Many cops that deal with alignment don't work well with tabs, and many times it's a problem that can't be solved unless we mix tabs and spaces, and then what's the point of using tabs?

@bbatsov bbatsov closed this Aug 4, 2022
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.

None yet

5 participants