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

Support option to only report lines that have changed #219

Closed
mehagar opened this issue Jun 20, 2020 · 13 comments
Closed

Support option to only report lines that have changed #219

mehagar opened this issue Jun 20, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@mehagar
Copy link

mehagar commented Jun 20, 2020

When linting files, it can be frustrating for a developer editing a file to have to fix all linting rules when they just edited one line.

I'd like for there to be an option to only report errors on lines that have changed.

@mehagar mehagar added the enhancement New feature or request label Jun 20, 2020
@thematchless
Copy link
Contributor

i thought the VALIDATE_ALL_CODEBASE: false would achieve this behaviour - i was wrong 😭

@nemchik
Copy link
Collaborator

nemchik commented Jul 6, 2020

This will be nearly impossible to achieve as it would be up to the individual lint tools to determine whether there is an issue or not and in order to parse the code and check validity they usually need to read the whole file. Here is an example of why:

Original code:

if (true) {
    console.log("Hello World!");
}

New code:

if (false) {
    console.log("Goodbye World!");
}

What has changed above?

if (true) {
    console.log("Hello World!");

Became

if (false) {
    console.log("Goodbye World!");

Notice in the changed code we have the opening curly brace { for the if statement, but we do not have the closing curly brace } because it has not changed. The linter would have no way to know that you have valid code without confirming that you have the closing curly brace by parsing the whole file. In that scenario it does not make sense to only lint the changed lines as the linter would have to be aware of what has changed (that's git's job, not the linter).

@thematchless
Copy link
Contributor

Oh wait a second ... i didn't had enough coffee this morning. I didn't read that @mehagar was looking to achieve this for the changed lines only. I thought he was looking for a way to lint only the files he had touched with a pull request. That was the reason i was looking for and couldn't get to run. But thanks for the explanation @nemchik 👍

@nemchik
Copy link
Collaborator

nemchik commented Jul 6, 2020

VALIDATE_ALL_CODEBASE: false I think should only lint changed files. It uses git diff I'm pretty sure. https://github.com/github/super-linter/blob/f948639bf18dcc7d9f20a3d1beb2964ef91fb252/lib/buildFileList.sh#L60

@thematchless
Copy link
Contributor

Interesting! I am not quite sure if i hadn't merged the latest changes from develop into my pull request and therefore got more changes while linting but i changed the DEFAULT_BRANCH from develop to origin/develop and now it seems to be working. Thank you a lot @nemchik 🙏

@mehagar mehagar changed the title Support option to only check lines that have changed Support option to only report lines that have changed Jul 6, 2020
@mehagar
Copy link
Author

mehagar commented Jul 6, 2020

This will be nearly impossible to achieve as it would be up to the individual lint tools to determine whether there is an issue or not and in order to parse the code and check validity they usually need to read the whole file. Here is an example of why:

Original code:

if (true) {
    console.log("Hello World!");
}

New code:

if (false) {
    console.log("Goodbye World!");
}

What has changed above?

if (true) {
    console.log("Hello World!");

Became

if (false) {
    console.log("Goodbye World!");

Notice in the changed code we have the opening curly brace { for the if statement, but we do not have the closing curly brace } because it has not changed. The linter would have no way to know that you have valid code without confirming that you have the closing curly brace by parsing the whole file. In that scenario it does not make sense to only lint the changed lines as the linter would have to be aware of what has changed (that's git's job, not the linter).

I understand what you mean. You're right - you would need to lint the entire file. But you could still only report on lines that have linting errors. If the linting tools report a line number, you could only pass that through the Github Linter if the line number was the same as a changed line in the PR.

I updated the PR's title to be "report" instead of "check".

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2020

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 Stale issue/pr label Aug 6, 2020
@assignUser
Copy link
Contributor

@mehagar It sounds like your frustration stems from using super-linter on files with lots of issues and fixing them. Imho the best way would be to run a local linter that is integrated into your IDE and makes fixing up multiple issues easy, looking something like this:
image
(lintr for R in VScode)

And use superlinter to assert that all commited code is lint free instead of using it "line-by-line".

@github-actions github-actions bot removed the O: stale 🤖 Stale issue/pr label Aug 19, 2020
@mehagar
Copy link
Author

mehagar commented Aug 20, 2020

@assignUser , a local linter would be part of the workflow to make it easy to fix issues so they don't get caught by any CI linter. But if a developer is making a one line change to a file, I only want them to have to fix linting issues on that one line. As it is, the super linter would report on the entire file instead of just the one changed line.

As an example of what I want, see reviewdog, which by default only reports on the lines that were changed in the PR.

@assignUser
Copy link
Contributor

I see, this would require a lot of changes as the ouput from the linters is not parsed in anyway at the moment. Your suggestion would probably go well with #151 were reviewdog was also referenced.

@peternewman
Copy link

But you could still only report on lines that have linting errors. If the linting tools report a line number, you could only pass that through the Github Linter if the line number was the same as a changed line in the PR.

It's possibly still more subtle than this @mehagar depending on what you actually want to achieve.

If the line you change/remove is the only usage of an import, then the act of changing/removing that line would generate a lint warning at the top of the file about an unused import, but your suggested fix would filter that out. Obviously these are a different class of warnings to be considered.

Flagging alerts due to deletions may be rather challenging too, e.g. removing a blank line so there isn't enough whitespace between two functions. Or even inserting a new function in between two existing ones and therefore reducing the amount of whitespace too much.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 Stale issue/pr label Sep 29, 2020
@pheirendt-jama
Copy link

This plugin claims to do what you are asking for. I have not tested it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants