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

Problem with autocorrect Style/Tab and Style/IndentationConsistency #1928

Closed
obromios opened this issue May 30, 2015 · 9 comments
Closed

Problem with autocorrect Style/Tab and Style/IndentationConsistency #1928

obromios opened this issue May 30, 2015 · 9 comments
Assignees
Labels

Comments

@obromios
Copy link

When I run rubocop --autocorrect on my project with Style/Tab and Style/IndentationConsistency enabled I get many errors, due to rubocop splitting up words. A boiled down example file is:

require 'spec_helper'
describe ArticlesController do
  render_views
    describe "GET 'index'" do
            it "returns http success" do
            end
        describe "admin user" do
             before(:each) do
            end
        end
    end
end

The output is as follows:

require 'spec_helper'
describe ArticlesController do
  render_views
  describe "GET 'index'" do
          it "returns http success" do
          end
          describe "admin user" d    o
           before(:each) d    o
           en    d
      end
  end
end

Note the split do after the before(:each). It not only splits up keywords, but symbols, literals, i.e. it seems to do it pretty much everywhere.

@obromios obromios changed the title Bug with autocorrect Style/Tab and Style/IndentationConsistency Problem with autocorrect Style/Tab and Style/IndentationConsistency May 30, 2015
@jonas054 jonas054 added the bug label May 31, 2015
@jonas054 jonas054 self-assigned this May 31, 2015
bbatsov added a commit that referenced this issue Jun 5, 2015
…nd_tabs

[Fix #1928] Auto-correct one offense at a time if there are tabs
@obromios
Copy link
Author

Hi
I have updated to 0.32, and bundle does indeed show
Using rubocop 0.32.0

If I copy the input code from above and paste into a new file, temp_spec.rb and run

`be rubocop --auto-correct spec/temp_spec.rb``
then I get exactly the same output as reported above, i.e. it breaks the code. Am I doing something wrong here?

@jonas054
Copy link
Collaborator

@obromios I get the same result. Sorry about that. I didn't check that my fix resolves the original problem. I'll take another look.

@jonas054 jonas054 reopened this Jun 11, 2015
@obromios
Copy link
Author

That's alright, I am sorry I have such messy code.

On Friday, June 12, 2015, Jonas Arvidsson notifications@github.com wrote:

@obromios https://github.com/obromios I get the same result. Sorry
about that. I didn't check that my fix resolves the original problem. I'll
take another look.


Reply to this email directly or view it on GitHub
#1928 (comment).

@lumeet
Copy link
Contributor

lumeet commented Jun 12, 2015

I stumbled upon a similar issue with IndentationConsistency. Here's a shortened version of the failing case:

def method
  a = 1
    if something
      if a
        b

      if test.nil?
      end
      end
    end
end

@jonas054
Copy link
Collaborator

Yes, the problem is not directly related to tab characters. Rather it's the fact that cops that deal with alignment typically change more than one offending line in autocorrect. They change the alignment of a whole range of lines. If there are other offenses within the realigned line range, then bad things happen. I'm working on a solution.

bbatsov added a commit that referenced this issue Jun 16, 2015
[Fix #1928] Avoid two changes overwriting in AutocorrectAlignment
@obromios
Copy link
Author

The latest patch has improved things, but I regret I am still having trouble. Here is an example of what is going wrong:

require 'spec_helper'
require 'pp'
describe Admin::AccountsController do
  render_views

    describe 'access control' do
      it 'must be signed in' do
        get :index
        response.should redirect_to signin_path
      end
      context 'signed_in' do
        let!(:user) { setup_and_sign_in_user }
        let!(:account) { user.account }
      it 'rejects ordinary user' do
        get :index
        response.should redirect_to root_path
      end
      it 'admin user' do
        user.toggle(:admin)
        get :index
        h1_is_correct 'Admin for Accounts'
        response.should render_template('admin/accounts/index')
      end
      end
    describe 'methods' do
      let!(:user) { setup_and_sign_in_user }
      let!(:account) { user.account }
      before(:each) { user.toggle(:admin) }
      describe :edit do
        it 'should open right page' do
          get :edit, id: user
          h1_is_correct 'Editing Account'
        end
        it 'Sets right value to set_free_days' do
          get :edit, id: account
          assigns(:free_days_left).should eq 7
        end
      end
      describe :update do
        it 'should allow changes' do
          get :update, id: account, account: { paid_up: true, set_free_days: 0 }
          account.reload
          account.paid_up.should be_true
          account.free_period.should eq 7
        end
        it 'should allow free days setting' do
          get :update, id: account, account: { set_free_days: 11 }
          account.reload
          account.free_period.should eq 11
        end
      end
    end
    end
end

is ending up as

require 'spec_helper'
require 'pp'
describe Admin::AccountsController do
  render_views

  describe 'access control' do
    it 'must be signed in' do
      get :index
      response.should redirect_to signin_path
    end
    context 'signed_in' do
      let!(:user) { setup_and_sign_in_user }
      let!(:account) { user.account }
      it 'rejects ordinary user' d  o
      get :inde  x
      response.should redirect_to root_pat  h
    end
      it 'admin user' d  o
      user.toggle(:admin  )
      get :inde  x
      h1_is_correct 'Admin for Accounts  '
      response.should render_template('admin/accounts/index'  )
    end
    end
    describe 'methods' d  o
    let!(:user) { setup_and_sign_in_user   }
    let!(:account) { user.account   }
    before(:each) { user.toggle(:admin)   }
    describe :edit d  o
      it 'should open right page' d  o
        get :edit, id: use  r
        h1_is_correct 'Editing Account  '
      en  d
      it 'Sets right value to set_free_days' d  o
        get :edit, id: accoun  t
        assigns(:free_days_left).should eq   7
      en  d
    en  d
    describe :update d  o
      it 'should allow changes' d  o
        get :update, id: account, account: { paid_up: true, set_free_days: 0   }
        account.reloa  d
        account.paid_up.should be_tru  e
        account.free_period.should eq   7
      en  d
      it 'should allow free days setting' d  o
        get :update, id: account, account: { set_free_days: 11   }
        account.reloa  d
        account.free_period.should eq 1  1
      en  d
    en  d
  end
  end
end

and

module PlansHelper
  def for_student(student)
    " for #{student.name}" if student.present?
  end

    def handicap_country_list
      [['No Handicap', 0], ['Australian', 1], ['United States', 2], ['New Zealand', 3], ['South African', 4], ['United Kingdom', 5], ['Canadian', 6], ['Other', 7]]
    end

    def durationList
      dList = []
    dList[0] = ['0 hours', 0]
    dList[1] = ['1 hour', 60]
    for i in 2..10
      dList[i] = [i.to_s + ' hours', i * 60]
    end
    nExtra = 9
    nStart = dList.size - 1
    for i in 1..nExtra do
      dList[i + nStart] = [(i * 5 + 10).to_s + ' hours', i * 10 * 60]
    end
    dList[dList.size] = [(nExtra * 10).to_s + '+ hours', nExtra + 1]
    dList
    end

    def lesson_list
      (0..20).to_a
    end
end

is ending up as

module PlansHelper
  def for_student(student)
    " for #{student.name}" if student.present?
  end

  def handicap_country_list
    [['No Handicap', 0], ['Australian', 1], ['United States', 2], ['New Zealand', 3], ['South African', 4], ['United Kingdom', 5], ['Canadian', 6], ['Other', 7]]
  end

  def durationList
    dList = []
    dList[0] = ['0 hours', 0]
    dList[1] = ['1 hour', 60]
    for i in 2..1  0
    dList[i] = [i.to_s + ' hours', i * 60  ]
  end
    nExtra = 9
    nStart = dList.size - 1
    for i in 1..nExtra d  o
    dList[i + nStart] = [(i * 5 + 10).to_s + ' hours', i * 10 * 60  ]
  end
    dList[dList.size] = [(nExtra * 10).to_s + '+ hours', nExtra + 1]
    dList
  end

  def lesson_list
    (0..20).to_a
  end
end

@jonas054
Copy link
Collaborator

@obromios Are you sure? I get exactly the results you describe when I run on v0.32.0 but on v0.32.1 and on master I see no problems.

@obromios
Copy link
Author

I will give that a go.

Chris

On Tue, Jul 28, 2015 at 5:59 PM, Jonas Arvidsson notifications@github.com
wrote:

@obromios https://github.com/obromios Are you sure? I get exactly the
results you describe when I run on v0.32.0 but on v0.32.1 and on master I
see no problems.


Reply to this email directly or view it on GitHub
#1928 (comment).

@obromios
Copy link
Author

... and it worked. It is strange though, I had checked rubygems and it showed 0.32.1 as being released.

Anyway, my code now looks beautiful and the test for that particular file all passed. I will now try rubocop --autocorrect on the whole app and get back to you if there is a problem.

Thank you.

alexdowad added a commit to alexdowad/rubocop that referenced this issue Jan 10, 2016
In e0fce7e, jonas054 adjusted autocorrection to only correct a single offense
at a time (rather than all offenses for a single cop) if the source file
contains tabs.

The comments indicate that autocorrecting files which contains tabs may be
problematic somehow, but it's hard to see how that is the case. All specs,
including the specs which were added in e0fce7e, still pass when the added
code is removed.

What prompted the addition of that code in the first place? It was made in
response to GH issue rubocop#1928. BUt all the "problem" code which was posted in that
thread is still autocorrected without problems, even when this "fix" is removed.

Why is removing this code desirable? Because, simply put, it flushes
autocorrection performance right down the toilet, and straight into the sewer.
It's hard to overstate how bad it is for perf. It takes what was just mildly
terrible and makes it appallingly, disastrously horrendous. And when I say
"horrendous", what I mean is "hideously atrocious".

If we still have trouble with tabs, it would be better to do a pass which
replaces tabs first, before any other cops get a crack at autocorrection. But
it remains to be seen whether that is actually necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants