Check filtered finding when error is on another line than git diff

[ruudk]

I'm trying out to create a PHP Linter.

I have a file like this:

public function create()
{
    // i make a mistake below by forgetting the trailing ;  line 33
    blaat()  // <-- line 34
}

My php linter gives the following error:

src/AdminBundle/Job/PostAdminMessageToSlackJob.php:35: PHP Parse error: syntax error, unexpected '$job' (T_VARIABLE)

And Reviewdog marks this finding as filtered:

Findings (0)
Filtered Findings (1)
src/AdminBundle/Job/PostAdminMessageToSlackJob.php:35: PHP Parse error: syntax error, unexpected '$job' (T_VARIABLE)

That's because Reviewdog does a strict lookup for the given row number in the git diff. But in PHP the error will sometimes be on the next line.

Any idea how to solve this?

Doesn't Github allow for adding checks to lines that didn't change? It always shows ±5 lines before and after right? Maybe we can take that margin into account here?

[BattleForged]

I encountered a similar problem when using flake8.
Just like

+ import yaml
  import os # H306 imports not in alphabetical order
  import yaml # F811 redefinition of unused

[haya14busa]

Now, you can use -reporter=github-check to report changes outside diff.

Maybe we can take that margin into account here?

It could be interesting to implement for github-pr-review reporter, but it's too specific issue for github review API, so I don't have an immediate plan to implement it.

[Le6ow5k1]

@haya14busa usage of -reporter=github-check on a pull request is problematic as it's going to report errors for the whole repository.
The ability to see the errors only for the PR's diff is super convenient but the above limitation makes it a bit less appealing.

I would like to implement such feature, maybe you have some thoughts on that?

I think the github-pr-review and github-pr-check reporters can take into account not only the added lines but all lines that are in diff (including the margins of unchanged lines). WDYT?

[haya14busa]

On second thought, it sounds interesting.
If we use all lines in diff instead of arbitrary margin number, it sounds simple enough and GitHub Review API should always work.

@Le6ow5k1 can you implement it?
It could be default behavior, but I prefer to adding a flag to turn on/off this behavior.

[Le6ow5k1]

@haya14busa yes, I will

[beevee]

I'm not sure whether my problem relates to this issue or not. So I decided to ask here first, before creating a new issue.

I'm using:

• golangci-lint linter
• github-pr-check reporter
• git diff master as a diff command

For this configuration, I can easily create a PR with a new linter error that reviewdog will miss. Let's say I have a constant that is defined in one file (with all other constants) and used only once in another place. Removing this single usage will lead to unused variable linter error, but this error will appear in a file that is not even a part of the PR. So git diff master won't contain this file. So reviewdog will not report this.

See this PR for a real-life example: moira-alert/moira#468

@haya14busa @Le6ow5k1 am I missing something? Is it possible to run reviewdog on the entire codebase, not just diff? Assuming I have zero linter complaints for current master branch.

[Le6ow5k1]

@beevee
Yeah, it is possible to run reviewdog for the whole repository using -reporter=github-check option.

[beevee]

@Le6ow5k1 -reporter=github-check seems to work for us, thank you!

Is there a way to make local reporter to work without diff? Sure, I can run bare golangci-lint locally, but I'd like to preserve consistency between CI runs and local runs.

[Le6ow5k1]

@beevee

I believe currently there is no way to use local reporter without diff, you have to pass -diff command.

As a workaround you might try to specify diff between current and first commit git diff `git rev-list HEAD | tail -n 1`

[haya14busa]

Now you can use -filter-mode=nofilter for that purpose.
https://github.com/reviewdog/reviewdog#filter-mode

If you want, you can also use github-pr-review reporter as well with -filter-mode=nofilter and -fail-on-error to catch errors outside diff.

The new binary is not released yet, but I'll release it in a few days.

[haya14busa]

The new binary released: https://github.com/reviewdog/reviewdog/blob/master/CHANGELOG.md#v0100---2020-05-07