Pronto detects only some of the violations

[sauliusgrigaitis]

Pronto detects only some of the violations. I like the idea of quickly checking only 'relevant changes', but it simply doesn't work by design. If it's a known design flaw, then Readme should clearly say that. If it's not known - here are steps to reproduce it:

1. Take some Ruby method with max allowed length by your RuboCop config.
2. Add one more line to that method.
3. Run Pronto and RuboCop.

RuboCop will detect violation and mark it at the method definition line. Pronto will not find it as it only checks the diff and the method definition line is not in the diff.

Until this gets resolved, a (slower) correctly working solution is to directly run linters on Jenkins and report new violations via Violations Plugin or some other preferred way.

[mmozuras]

Using something like Violations Plugin is a valid alternative, definitely. Depends on your use case!

@sauliusgrigaitis I disagree with some but in general, you're right. While Pronto works for most violations, it doesn't work for every violation. But #221 is not the solution 🙂. Here are the options I see:

1. Working with linters creators that they would provide more than just the line number, but the range of lines for a violations. Some linters already do that, but we don't always take advantage. So, in the case you've wrote about: instead of providing the line number for the method definition, all the line numbers for the method would be provided. This would allow Pronto (and other similar tools) to work for these kind of violations too 🙂.

2. Documenting cases that don't work for each runner.

3. Adding a Known limitations section to the README. And yes, it should be more than a couple of words 🙂.

What do you think? What other alternatives do you see?

[mmozuras]

For reference, an instance of the same issue: https://github.com/mmozuras/pronto-scss/issues/7

/cc @ibrahima

[sauliusgrigaitis]

Well, it depends on the point of view. I think that those code structure related violations (methods complexity, length etc) are way more important that those Pronto are able to detect. That's why I use some. Wording for sure is a subjective thing, but I would not use word perfect at this stage :)

In any case, I strongly suggest to clearly point this issue in the Readme. A lot of folks may even not notice that a lot of important violations are not reported.

One of the ways to fix it would be to add different levels:

• Current way - works fast, detects only some of violations
• Runs linters on both current and target branch for all changed files. Compares violations and reports new valuations in current branch. A bit slower, finds almost all violations, doesn't detect similar code among separate files.
• Runs linters on both current and target branch for all files. Works slow, detects everything.

This approach doesn't depend on linters efforts to support such tools. I think it's the way to go for the short term.

[mmozuras]

@sauliusgrigaitis agree with wording being a subjective thing. And did not suggest to use perfect and never claimed it 😄

Would also point out that Pronto is able to detect those violations in many cases. Using your example, Pronto would detect that kind of violation if that method would be added as a whole or its signature would be changed.

In any case, I strongly suggest to clearly point this issue in the Readme.

👍

[mguterl]

We have similar issues and had to move away from pronto-rubocop because of it. We also ran into issues when we upgraded rubocop and new rules were introduced.

[mmozuras]

@mguterl what kind of issues did you run into after you upgraded rubocop? 🙂

[sauliusgrigaitis]

And did not suggest to use perfect and never claimed it...

Well, then I'm not sure what is it "...Perfect if you want to find out quickly if a branch introduces changes that conform..."

Would also point out that Pronto is able to detect those violations in many cases..

It doesn't change the fact that there are a lot of cases when it doesn't work. If it doesn't work at least in some of the cases it's not usable for me. Like most of the things in engineering.

Seriously, Readme update is a must. Entire files violations comparison would make Pronto usable and still fast enough (especially with caching implemented at least on target branch linting results).

[mmozuras]

Seriously, Readme update is a must.

Agreed. I've offered it as an option in my first comment and agreed to it with my third comment in this thread 🙂.

Entire files violations comparison would make Pronto usable and still fast enough (especially with caching implemented at least on target branch linting results).

Most of the time Pronto is used to make comments on pull-requests / merge-requests. While running Pronto on the whole file is possible (and easy), the harder part is: where to post those comments then if they are not part of the pull-request / merge-request diff? Thoughts? Ideas? How could we handle it? 🙂

@sauliusgrigaitis I would also like to ask you to keep the conversation constructive and help us find a solution. Posting memes and repeating the fact that it doesn't work for you is not that 😄

[sauliusgrigaitis]

where to post those comments then if they are not part of the pull-request / merge-request diff? Thoughts? Ideas? How could we handle it? 

Just like any other comments that are not dedicated to a particular line of the pull request, for example how this pull request affects entire codebase? There is usually common section something like Discussion section in GitLab.