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

Don't run pylint by default even if installed #155

Closed
ptrcnull opened this issue Jul 20, 2022 · 4 comments · Fixed by #156
Closed

Don't run pylint by default even if installed #155

ptrcnull opened this issue Jul 20, 2022 · 4 comments · Fixed by #156

Comments

@ptrcnull
Copy link

ptrcnull commented Jul 20, 2022

tests for 0.28.4 fail because mockobject.py was not linted correctly:

..............................................s.................************* Module dbusmock.mockobject
dbusmock/mockobject.py:355:0: R0022: Useless option value for 'disable', 'no-self-use' was moved to an optional extension, see https://pylint.pycqa.org/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)
dbusmock/mockobject.py:455:17: C3001: Lambda expression assigned to a variable. Define a function using the "def" keyword instead. (unnecessary-lambda-assignment)

------------------------------------------------------------------
Your code has been rated at 9.96/10

Es...............................ssssssssss.sssssssssssssssssss....ssssss.......ssss..sssssssssssss.....
======================================================================
ERROR: test_pylint (tests.test_code.StaticCodeTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_code.py", line 39, in test_pylint
    subprocess.check_call([sys.executable, '-m', 'pylint'] + glob.glob('dbusmock/*.py'))
  File "/usr/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/usr/bin/python3', '-m', 'pylint', 'dbusmock/__main__.py', 'dbusmock/__init__.py', 'dbusmock/mockobject.py', 'dbusmock/testcase.py']' returned non-zero exit status 24.

----------------------------------------------------------------------

i'm not really sure why pylint has to be launched from tests, but if you include lints in your testsuite, please check them before releasing a new version :p

@martinpitt
Copy link
Owner

I do check them -- they run in CI. But only on Fedora stable, not on Rawhide any more. Recently pylint changed behaviour wrt. that no-self-use thing, and there just isn't a way to make the code happy with both 2.13 and 2.14.

Where do you see this failure exactly? It's not in any of the distro builds (pylint should not run anywhere there, it changes too often) or upstream CI.

@martinpitt
Copy link
Owner

In other words, this can never be truly "fixed" -- if pylint changes to a newer version and changes rules, this will break all released versions. That's not really pylint's fault, it just means that it does not make much sense to run pylint on released versions.

Right now the unit tests merely check if pylint and friends are installed. I should probably change that to explicitly opt-in, so that my CI can do this in a way that doesn't break test runs for users?

@martinpitt martinpitt changed the title Check tests before tagging a release Don't run pylint by default even if installed Jul 21, 2022
@ptrcnull
Copy link
Author

thanks for the quick response!

there just isn't a way to make the code happy with both 2.13 and 2.14

ohh, okay, that makes way more sense now

Where do you see this failure exactly?

running python3 setup.py test on Alpine Linux edge (pylint 2.14.4)

pylint should not run anywhere there

well, it's being picked up by the test runner and while it's not a huge problem while packaging for distributions (unless pytest is pulled in by something), it would be cool if lints were not included in the test suite by default

martinpitt added a commit that referenced this issue Jul 21, 2022
Running them merely when they are installed is a trap: We know that our
code does not satisfy the latest pylint release. The static code tests
are for upstream development, not for downstream or user-side testing.

So only run them when `$TEST_CODE` is set. Plumb that through the
container and the script, and set it in our GitHub workflow.

Fixes #155
martinpitt added a commit that referenced this issue Jul 21, 2022
Running them merely when they are installed is a trap: We know that our
code does not satisfy the latest pylint release. The static code tests
are for upstream development, not for downstream or user-side testing.

So only run them when `$TEST_CODE` is set. Plumb that through the
container and the script, and set it in our GitHub workflow.

Fixes #155
@martinpitt
Copy link
Owner

Fixed in PR #156

martinpitt added a commit that referenced this issue Jul 21, 2022
Running them merely when they are installed is a trap: We know that our
code does not satisfy the latest pylint release. The static code tests
are for upstream development, not for downstream or user-side testing.

So only run them when `$TEST_CODE` is set. Plumb that through the
container and the script, and set it in our GitHub workflow.

Fixes #155
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 a pull request may close this issue.

2 participants