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

"Use next to skip iteration" reporting wrong location. #2138

Closed
zenspider opened this issue Aug 12, 2015 · 4 comments · Fixed by #2191
Closed

"Use next to skip iteration" reporting wrong location. #2138

zenspider opened this issue Aug 12, 2015 · 4 comments · Fixed by #2191

Comments

@zenspider
Copy link

Related to #1238:

def xxx
  loop do
    do_stuff

    unless condition then
      do_other_stuff1
      do_other_stuff2
      do_other_stuff3
    end
  end
end

reports:

Offenses:

wtf.rb:2:3: C: Use next to skip iteration.
  loop do
  ^^^^

But, the loop on line 2 column 3 is not the problem. The problem is in the contents of the loop, on line 5, column 4. This makes the report confusing and misleading.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 12, 2015

Any suggestions about the position of the offence and its message?

@zenspider
Copy link
Author

I would expect the ^^^ part to be pointing at the jumping construct (if/unless), not the looping construct.

@rrosenblum
Copy link
Contributor

I agree with @zenspider on this one. The error should be pointing to the location of the offense (unless condition then). If we move the the offense to point to the condition, then we can have the cop call out multiple offense in a given loop.

Example

def xxx 
  loop do
    do_stuff

    unless condition then
      do_other_stuff1
      do_other_stuff2
      do_other_stuff3
    end 

    unless condition2 then
      do_other_stuff4
      do_other_stuff5
      do_other_stuff6
    end 
  end 
end

reports

test.rb:2:3: C: Use next to skip iteration.
  loop do
  ^^^^

With the current implementation, this could lead to confusion for someone trying to fix what they think is only one issue, but in actuality is two issues.

@rrosenblum
Copy link
Contributor

I just started working on this cop. I realized that the example that I wrote earlier only contains 1 offense, not 2 like I originally was thinking. I am still going to work on moving the highlight.

rrosenblum added a commit to rrosenblum/rubocop that referenced this issue Aug 26, 2015
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 a pull request may close this issue.

3 participants