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

pylintc for new code: disable format checker #1659

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

MVrachev
Copy link
Collaborator

@MVrachev MVrachev commented Nov 2, 2021

Description of the changes being introduced by the pull request:

By default pylint does format checks:
https://pylint.pycqa.org/en/latest/technical_reference/features.html?highlight=format#format-checker

The problem is we also use black and isort who have format checkers as
well. This makes pylint format checks obsolete.

Also, it's possible that you would want to disable a warning and you
can end up in the situation where you will have to disable it for
two tools altogether.

Signed-off-by: Martin Vrachev mvrachev@vmware.com

By default pylint does format checks:
https://pylint.pycqa.org/en/latest/technical_reference/features.html?highlight=format#format-checker

The problem is we also use black and isort who have format checkers as
well. This makes pylint format checks obsolete.

Also, it's possible that you would want to disable a warning and you
can end up in the situation where you will have to disable it for
two tools altogether.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Copy link
Collaborator Author

MVrachev commented Nov 2, 2021

As Jussi has pointed out, even the pylint project had disabled format for pylint:
https://github.com/PyCQA/pylint/blob/main/pylintrc#L80 😆.

@coveralls
Copy link

coveralls commented Nov 2, 2021

Pull Request Test Coverage Report for Build 1425820471

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 97.442%

Totals Coverage Status
Change from base Build 1390970499: 0.01%
Covered Lines: 3946
Relevant Lines: 4030

💛 - Coveralls

@MVrachev
Copy link
Collaborator Author

MVrachev commented Nov 2, 2021

I am finding more and more checks for pylint that overlap with what black
I found one more check that overlaps with black work inconsistent-quotes
and another one that overlaps with what isort does wrong-import-order

@jku do you think we should disable those checks?

@jku
Copy link
Member

jku commented Nov 3, 2021

If the checks are problematic, then sure why not. But let's not disable things if they're working fine

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this:

  • black seems to do a good job of format enforcement
  • if we need to add local disables (like we'd like to in some test code), having two checkers for the same thing is problematic

@MVrachev
Copy link
Collaborator Author

MVrachev commented Nov 3, 2021

If the checks are problematic, then sure why not. But let's not disable things if they're working fine

Locally I noticed a couple of warnings from pylint when I wanted to prepare a pr for pylint.
I will wait to have #1658 merged and then propose a pr for pylint.
If those warnings are still visible then I will disable them there.

@jku
Copy link
Member

jku commented Nov 3, 2021

Now that I think about it... you could remove the format checker configuration section from pylintrc as well: if it does not get used, it should not be there

The "FORMAT" section in pylint is no longer needed after the format
checker is disabled.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@jku jku merged commit 8ae944c into theupdateframework:develop Nov 5, 2021
@MVrachev MVrachev deleted the disable-pylint-format branch November 5, 2021 15:50
@lukpueh lukpueh mentioned this pull request Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants