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

Line numbers reported by linters do not correspond to the committed files #3399

Open
vkucera opened this issue Feb 28, 2024 · 15 comments
Open
Labels
enhancement New feature or request

Comments

@vkucera
Copy link
Contributor

vkucera commented Feb 28, 2024

Is your feature request related to a problem? Please describe.

Line numbers reported by linters do not correspond to the committed files, probably as a result of formatting by the formatters running before them.

Describe the solution you'd like

Line numbers referred in the reports of linters should be the numbers of lines where the errors appear in the committed files.

Describe alternatives you've considered

If formatters run after the linters, the line numbers of errors found by linters will be correct.

Additional context

@vkucera vkucera added the enhancement New feature or request label Feb 28, 2024
@nvuillam
Copy link
Member

When linters are in the same descriptor, the linters that can update (formatters,linters with auto-fix) are always run before the others

But... with "generic" formatters that's probably not the case...

Do you have concrete examples in mind ?

@vkucera
Copy link
Contributor Author

vkucera commented Mar 22, 2024

Yes, the Python linters seem to run in the following order:

  1. black
  2. isort
  3. ruff
  4. pylint
  5. mypy
  6. pyright
  7. flake8
  8. bandit

If black, isort and ruff format the files on the fly and change the line numbering, line numbers reported by pylint refer to the formatted files and don't match to lines in the committed files anymore.
IMHO, this is an undesired behaviour which makes finding the problems in the code more confusing.

@vkucera
Copy link
Contributor Author

vkucera commented Mar 22, 2024

Besides, another benefit of running the formatters last would be the possibility to speedup MegaLinter's running time by skipping the formatters if some of the linters have failed.

@echoix
Copy link
Collaborator

echoix commented Mar 22, 2024

But on the other hand, some easily fixed issues, like line length, types of quotes etc, that can be fixed by a formatter without failing, will make the linters fail afterwards. Some of these tools are slower too.

@vkucera
Copy link
Contributor Author

vkucera commented Mar 22, 2024

But on the other hand, some easily fixed issues, like line length, types of quotes etc, that can be fixed by a formatter without failing, will make the linters fail afterwards. Some of these tools are slower too.

Which non-formatting linter can fail because of a formatting issue?

@nvuillam
Copy link
Member

That's the chicken or the egg problem... :)

If we don't run linters that can fix first, the following linters will find issues that would have been fixed before by the formatters/fixers...

In case of failure, formatted/fixed files are available in artifacts, so they can be copy-pasted locally to apply the fixes anyway, so have the correct number of lines

That's far from ideal but I think it has less impacts to use such workaround than to change MegaLinter automated execution order

@vkucera
Copy link
Contributor Author

vkucera commented Mar 27, 2024

I would agree with you if the non-formatting linters could fail because of a formatting issue (hence my last, yet unanswered, question). But I don't think that is the case. Do you have an example?

@echoix
Copy link
Collaborator

echoix commented Mar 27, 2024

Flake8 for sure, Pylint also, ruff implements checks for some of these rules too, so that's just the top of my head of thing I saw elsewhere last week when upgrading a codebase from black 22.3 to 24.3

@vkucera
Copy link
Contributor Author

vkucera commented Mar 27, 2024

OK, if you say that formatting by Black can fix something that would make Pylint fail, then I agree that it makes sense to run the formatters first.

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 🤖 This issue or pull request is stale, it will be closed if there is no activity label Apr 27, 2024
@vkucera
Copy link
Contributor Author

vkucera commented May 10, 2024

I am now experiencing a situation where yamllint runs before prettier and fails because of formatting issues which are later fixed by prettier. So, although you convinced me that formatters should run first, it does not seem to be the case which points to another problem. Could it be that yamllint and prettier run in parallel jobs and yamllint is just faster?

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label May 11, 2024
@nvuillam
Copy link
Member

@vkucera I think a local run of prettier and a commit in your repo should solve this mess for a while ^^

Of just make yamllint non blocking by adding YAML_TAMLLINT in DISABLE_ERRORS_LINTERS property

@vkucera
Copy link
Contributor Author

vkucera commented May 16, 2024

@nvuillam thanks, that can work as a workaround but, since you said that formatters always run before other linters, I am wondering whether the case of yamllint running before prettier is a feature or a bug.

@nvuillam
Copy link
Member

@vkucera the algorith is the following

def process_linters_parallel(self, active_linters, linters_do_fixes):

Groups of linters that can upate sources are run before groups of linters that can not update

There is nothing telling "wait for linters that can fix to run before running linters that can not fix", maybe this is the bug you suspect ?

@vkucera
Copy link
Contributor Author

vkucera commented May 22, 2024

@vkucera the algorith is the following

def process_linters_parallel(self, active_linters, linters_do_fixes):

Groups of linters that can upate sources are run before groups of linters that can not update

But this is apparently not true. If it was the case, it would be impossible to see yamllint (which cannot update sources) running before prettier (the only enabled linter which can update sources).

2024-05-10T11:13:56.2279237Z ##[group]�[31m❌ Linted [YAML] files with [yamllint]: Found 1 error(s) - (0.27s)�[0m (expand for details)
2024-05-10T11:13:56.2364557Z ##[endgroup]
2024-05-10T11:13:56.6316125Z ##[group]�[32m✅ Linted [YAML] files with [prettier] successfully - (0.6s)�[0m (expand for details)
2024-05-10T11:13:56.6345647Z ##[endgroup]
2024-05-10T11:13:59.5020765Z ##[group]�[32m✅ Linted [YAML] files with [v8r] successfully - (3.16s)�[0m (expand for details)
2024-05-10T11:13:59.5054149Z ##[endgroup]

There is nothing telling "wait for linters that can fix to run before running linters that can not fix", maybe this is the bug you suspect ?

If the linters that cannot update sources are not starting after the formatters in the given format group finish, I would indeed consider that to be a major bug. If the point of formatting is to avoid later failures of other linter (as @echoix pointed out), then it would be incorrect to run the non-formatting linters on files which have not been formatted yet.

Am I missing anything?

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

3 participants