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

New Reek violations may not be reported #20

Open
mjacobus opened this issue Mar 15, 2018 · 5 comments
Open

New Reek violations may not be reported #20

mjacobus opened this issue Mar 15, 2018 · 5 comments
Labels

Comments

@mjacobus
Copy link
Contributor

Given I have the code on GH (path/some_class.rb)

class SomeClass
  def do_stuff
    One.new
    Two.new
    Three.new
    Four.new
    Five.new
  end
end

The current reek status is:

Inspecting 1 file(s):
.

I change the file and create a pull request that changes the file like follows

class SomeClass
  def do_stuff
    One.new
    Two.new
    Three.new
    Four.new
    Five.new
    Six.new
  end
end
Inspecting 1 file(s):
S

/path/pronto_reek.rb -- 1 warning:
  [2]:TooManyStatements: SomeClass#do_stuff has approx 6 statements [https://github.com/troessner/reek/blob/master/docs/Too-Many-Statements.md]

Pronto rubocop will not create any comment on my PR, because the report is on
the method on line 2, which is not part of the changed code.

Now, if I add a comment in the line of the method name (where reek reports the
violaiton) and update the PR, I will get a comment from pronto, because line num 2 changed.

class SomeClass
  def do_stuff # now it reports
    One.new
    Two.new
    Three.new
    Four.new
    Five.new
    Six.new
  end
end

It is a bug, isn't it? The new code created a violation in the existing code.

@grantspeelman
Copy link

+1 to this idea/bug. Would be great if pronto-reek could cater for this somehow.

@mjacobus
Copy link
Contributor Author

I suppose a potential valid approach for a fix would be to compare the violations of the files before the PR changes with the ones gotten from each commit. The new violations should be the ones introduced by the PR.

@grantspeelman
Copy link

grantspeelman commented Mar 15, 2018

yes, but what line would it put the comment on? what happens if the correct line becomes available later on the PR later, Will it move the comment or leave it as it? Does pronto even support moving comments ....

@mjacobus
Copy link
Contributor Author

Good questions. Perhaps it would not comment on the diff line, if not possible but in the PR regular thread. Perhaps there is no perfect solution.

@ashkulz
Copy link
Member

ashkulz commented Feb 1, 2021

This is similar to prontolabs/pronto-rubocop#46 -- I suppose a common solution will work for both 🤔

@ashkulz ashkulz added the bug label Feb 1, 2021
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