search

bit-team / backintime

Back In Time - An easy-to-use backup tool for GNU Linux using rsync in the back
https://backintime.readthedocs.io
GNU General Public License v2.0
1.9k stars 175 forks source link

Enable PyLint rules of your own choice and fix the errors #1755

Open buhtz opened 3 weeks ago

buhtz commented 3 weeks ago

Introduction

Welcome to the project, if you pick up this Issue because of the "GOOD FIRST USE" label. You will be mentored through the process if you want. First the Issue is explained. At the end you will find some guidance for first contributors. Please do not hesitate to ask questions. Your solution don't need to be perfect.

Be aware that this is a meta Issue not describing one specific problem and task, but a bunch of similar problems you can choose from.

It is a fine issue for beginners.

Problem

Code analyzing tools like PyLint and others show multiple of 1.000 errors (~4.500 in default configuration) on the current code base of Back In Time. They can not be fixed all at once. Back In Time currently use pylint in two unit test (see common/test/test_lint.py and qt/test/test_lint.py) with a very small selection of enabled error codes.

In the code you find the enabled error codes as well the potential disabled codes. Have an eye on the disabled ones, starting with C0303: https://github.com/bit-team/backintime/blob/51ff9cfe56cacb19d6968dd586014608a34cb21f/common/test/test_lint.py#L98-L114

Be aware that there are two files named test_lint.py, one in folder common/test and one in qt/test. They should be identical, especially the list of enabled/disabled error codes.

Solution

Pick one of the disabled error codes listed in the source code linked above. Have a look at the PyLint documentation to read about that error code and how to fix it. You don't need to stick to the existing list of error codes. You are free to choose another one if you are able to fix it. Important: Pick one error only and work on one file only. Don't blow up the Pull Request.

Your next steps

  1. If this is your first contribution in this project please introduce your self and tell us about your skills, wishes and plans. Also let us know how you found the issue and the project.
  2. Read the existing contributors documentation.
  3. We can develop the next steps in the further discussion. Don't hesitate to ask.
  4. If you created a separate branch on your forked repo enable the choosen error code via uncommenting the specific line in the two test_lint.py files.
  5. Then run the test: Navigate into common and/or qt and run pytest test/test_lint.py. Work on that errors but only on one of the source files per PullRequest.
  6. Be careful! The job might look like easy but has a high potential introducing new bugs and problems.

Related code

Additional information

To get an idea about the errors pylint is giving for the whole codebase.

$ pylint --max-line-length=79 --good-names=idx,fp --extension-pkg-allow-list=PyQt6,PyQt6.QtCore --additional-builtins=_,ngettext --score n --reports y --recursive y qt common

Messages
--------

+---------------------------------+------------+
|message id                       |occurrences |
+=================================+============+
|invalid-name                     |1512        |
+---------------------------------+------------+
|line-too-long                    |948         |
+---------------------------------+------------+
|missing-function-docstring       |733         |
+---------------------------------+------------+
|consider-using-f-string          |407         |
+---------------------------------+------------+
|wrong-import-position            |120         |
+---------------------------------+------------+
|unspecified-encoding             |117         |
+---------------------------------+------------+
|super-with-arguments             |93          |
+---------------------------------+------------+
|unused-argument                  |77          |
+---------------------------------+------------+
|missing-class-docstring          |74          |
+---------------------------------+------------+
|consider-using-with              |69          |
+---------------------------------+------------+
|fixme                            |59          |
+---------------------------------+------------+
|missing-module-docstring         |49          |
+---------------------------------+------------+
|no-else-return                   |48          |
+---------------------------------+------------+
|broad-exception-caught           |48          |
+---------------------------------+------------+
|protected-access                 |47          |
+---------------------------------+------------+
|wrong-import-order               |42          |
+---------------------------------+------------+
|unused-variable                  |32          |
+---------------------------------+------------+
|inconsistent-return-statements   |30          |
+---------------------------------+------------+
|unused-import                    |28          |
+---------------------------------+------------+
|redefined-outer-name             |24          |
+---------------------------------+------------+
|unnecessary-lambda-assignment    |22          |
+---------------------------------+------------+
|bare-except                      |21          |
+---------------------------------+------------+
|too-many-statements              |20          |
+---------------------------------+------------+
|import-outside-toplevel          |20          |
+---------------------------------+------------+
|attribute-defined-outside-init   |19          |
+---------------------------------+------------+
|useless-object-inheritance       |18          |
+---------------------------------+------------+
|too-many-branches                |18          |
+---------------------------------+------------+
|too-many-instance-attributes     |16          |
+---------------------------------+------------+
|too-many-arguments               |16          |
+---------------------------------+------------+
|cyclic-import                    |16          |
+---------------------------------+------------+
|too-many-public-methods          |14          |
+---------------------------------+------------+
|too-many-locals                  |14          |
+---------------------------------+------------+
|empty-docstring                  |13          |
+---------------------------------+------------+
|duplicate-code                   |12          |
+---------------------------------+------------+
|too-many-lines                   |10          |
+---------------------------------+------------+
|no-else-raise                    |8           |
+---------------------------------+------------+
|dangerous-default-value          |7           |
+---------------------------------+------------+
|wildcard-import                  |6           |
+---------------------------------+------------+
|unused-wildcard-import           |6           |
+---------------------------------+------------+
|arguments-differ                 |6           |
+---------------------------------+------------+
|unnecessary-lambda               |5           |
+---------------------------------+------------+
|super-init-not-called            |5           |
+---------------------------------+------------+
|unnecessary-dunder-call          |4           |
+---------------------------------+------------+
|too-many-return-statements       |4           |
+---------------------------------+------------+
|singleton-comparison             |4           |
+---------------------------------+------------+
|redefined-builtin                |4           |
+---------------------------------+------------+
|raise-missing-from               |4           |
+---------------------------------+------------+
|expression-not-assigned          |4           |
+---------------------------------+------------+
|unnecessary-pass                 |3           |
+---------------------------------+------------+
|too-many-nested-blocks           |3           |
+---------------------------------+------------+
|too-few-public-methods           |3           |
+---------------------------------+------------+
|keyword-arg-before-vararg        |3           |
+---------------------------------+------------+
|consider-using-dict-items        |3           |
+---------------------------------+------------+
|consider-iterating-dictionary    |3           |
+---------------------------------+------------+
|arguments-renamed                |3           |
+---------------------------------+------------+
|useless-return                   |2           |
+---------------------------------+------------+
|useless-parent-delegation        |2           |
+---------------------------------+------------+
|use-implicit-booleaness-not-len  |2           |
+---------------------------------+------------+
|use-a-generator                  |2           |
+---------------------------------+------------+
|unnecessary-negation             |2           |
+---------------------------------+------------+
|try-except-raise                 |2           |
+---------------------------------+------------+
|too-many-ancestors               |2           |
+---------------------------------+------------+
|subprocess-popen-preexec-fn      |2           |
+---------------------------------+------------+
|no-else-continue                 |2           |
+---------------------------------+------------+
|global-variable-undefined        |2           |
+---------------------------------+------------+
|consider-using-sys-exit          |2           |
+---------------------------------+------------+
|chained-comparison               |2           |
+---------------------------------+------------+
|access-member-before-definition  |2           |
+---------------------------------+------------+
|used-before-assignment           |1           |
+---------------------------------+------------+
|unidiomatic-typecheck            |1           |
+---------------------------------+------------+
|ungrouped-imports                |1           |
+---------------------------------+------------+
|undefined-loop-variable          |1           |
+---------------------------------+------------+
|simplifiable-if-statement        |1           |
+---------------------------------+------------+
|simplifiable-if-expression       |1           |
+---------------------------------+------------+
|reimported                       |1           |
+---------------------------------+------------+
|redundant-unittest-assert        |1           |
+---------------------------------+------------+
|pointless-string-statement       |1           |
+---------------------------------+------------+
|pointless-statement              |1           |
+---------------------------------+------------+
|no-name-in-module                |1           |
+---------------------------------+------------+
|no-else-break                    |1           |
+---------------------------------+------------+
|global-variable-not-assigned     |1           |
+---------------------------------+------------+
|global-statement                 |1           |
+---------------------------------+------------+
|eval-used                        |1           |
+---------------------------------+------------+
|consider-using-set-comprehension |1           |
+---------------------------------+------------+
|consider-using-in                |1           |
+---------------------------------+------------+
|consider-using-get               |1           |
+---------------------------------+------------+
|consider-using-from-import       |1           |
+---------------------------------+------------+
|consider-using-enumerate         |1           |
+---------------------------------+------------+
|broad-exception-raised           |1           |
+---------------------------------+------------+
ayush571995 commented 2 weeks ago

Created pr : https://github.com/bit-team/backintime/pull/1759/files Need help to get this merged as first contribution to oss

ayush571995 commented 2 weeks ago

@buhtz can you please check the above pr ?

buhtz commented 2 weeks ago

Hello Ayush,

@buhtz can you please check the above pr ?

I am notified about all activities in this repository. So there is no need to ask again after 16 hours or directly mention me via using the @ sign with my account name. This produce extra noise and increase maintenance burden.

Kind Christian

vincentfar commented 6 days ago

Hello, my name is Vincent and I am new to Github open source contributions. For this issue, do I just select any error in test_lint.py and parse through any files within the entire program for that error and fix it? Kind Regards, Vincent Far

buhtz commented 6 days ago

Hello Vincent, welcome to the project and thank you for your efforts to contribute. Did you read our CONTIRBUTION.md file and understood everything?

As a first step I would say you fork the repo, create a separate branch and then try to run the test suite on your own system. When this works we can step further.

Best Christian

vincentfar commented 6 days ago

Hello Christian, Yes, I forked the repo and created a separate local branch. Ran the test_lint.py and it says "no test ran in 0.4 s". So would the next step be uncommenting the specific line in the two test_lint.py files?

buhtz commented 6 days ago

Ran the test_lint.py and it says "no test ran in 0.4 s".

That means something is wrong. How do you run the test? Can you post the command? See our doc about how to run tests. Be aware that we do have two test suites (common/test and qt/test) that need to be run one by one.

$ cd common
$ pytest test/test_lint.py

Or

$ cd common
$ python3 -m unittest test/test_lint.py

The output then should indicate that one test was run and passed.

So would the next step be uncommenting the specific line in the two test_lint.py files?

Yes. Then run the tests again and then see where the errors appear. Important: Keep the PullRequest small. No matter in how many files the error appears, fix only one file per PullRequest. You can exception that rule if you have 3 errors in 3 files for example. Decide this on your own. But keep the PR simple and small.

vincentfar commented 5 days ago

Hello, I ran $ cd common $ pytest test/test_lint.py

and the output was: ===================================================================== short test summary info ====================================================================== FAILED test/test_lint.py::MirrorMirrorOnTheWall::test_with_pylint - AssertionError: 0 != 54 : PyLint found 54 problems. ======================================================================== 1 failed in 13.47s ========================================================================

I see several import errors. I have read the Build & install and the dependencies section of the documentation. I believe I have installed ubuntu and the dependencies on my windows command prompt but I am unsure if this is the cause of the import errors. I also tried to uncomment specific lines such as "no-member / E1101" and "'R0201', # no-self-use" but there are no additional errors to the pylint test other than those original 54 errors. I apologize for my lack of experience! However, I'm not sure what I am doing wrong.

vincentfar commented 5 days ago

qt_probing.py:120:4: E0401: Unable to import 'PyQt6.QtWidgets' (import-error) Module snapshots snapshots.py:25:0: E0401: Unable to import 'pwd' (import-error) snapshots.py:27:0: E0401: Unable to import 'grp' (import-error) snapshots.py:410:20: E1101: Module 'os' has no 'chown' member (no-member) snapshots.py:420:20: E1101: Module 'os' has no 'chown' member (no-member) snapshots.py:1942:27: E1101: Module 'os' has no 'statvfs' member (no-member) snapshots.py:1982:19: E1101: Module 'os' has no 'statvfs' member (no-member) Module sshtools sshtools.py:327:54: E1101: Module 'signal' has no 'SIGKILL' member; maybe 'SIGILL'? (no-member) sshtools.py:420:51: E1101: Module 'os' has no 'setsid' member (no-member) sshtools.py:1171:39: E1101: Module 'os' has no 'setsid' member (no-member) Module tools tools.py:55:58: E1101: Module 'os' has no 'geteuid' member (no-member) tools.py:1630:11: E1101: Module 'os' has no 'geteuid' member (no-member) tools.py:2055:26: E1101: Module 'signal' has no 'SIGALRM' member (no-member) tools.py:2056:12: E1101: Module 'signal' has no 'alarm' member (no-member) tools.py:2066:12: E1101: Module 'signal' has no 'alarm' member (no-member) tools.py:2568:30: E1101: Module 'signal' has no 'SIGTSTP' member (no-member) tools.py:2569:30: E1101: Module 'signal' has no 'SIGCONT' member (no-member) tools.py:2570:30: E1101: Module 'signal' has no 'SIGHUP' member (no-member) tools.py:2621:30: E1101: Module 'signal' has no 'SIGTSTP' member (no-member) tools.py:2622:30: E1101: Module 'signal' has no 'SIGCONT' member (no-member) tools.py:2623:30: E1101: Module 'signal' has no 'SIGHUP' member (no-member) tools.py:2649:48: E1101: Module 'signal' has no 'SIGSTOP' member (no-member) tools.py:2658:48: E1101: Module 'signal' has no 'SIGCONT' member (no-member) tools.py:2698:18: E1101: Module 'os' has no 'fork' member (no-member) tools.py:2710:8: E1101: Module 'os' has no 'setsid' member (no-member) tools.py:2715:18: E1101: Module 'os' has no 'fork' member (no-member) tools.py:2814:25: E1101: Module 'signal' has no 'SIGHUP' member (no-member) Module test.test_config_crontab test\test_config_crontab.py:22:0: E0401: Unable to import 'pyfakefs.fake_filesystem_unittest' (import-error) Module test.test_restore test\test_restore.py:21:0: E0401: Unable to import 'pwd' (import-error) test\test_restore.py:22:0: E0401: Unable to import 'grp' (import-error) test\test_restore.py:32:13: E1101: Module 'os' has no 'geteuid' member (no-member) test\test_restore.py:35:13: E1101: Module 'os' has no 'getegid' member (no-member) Module test.test_snapshots test\test_snapshots.py:23:0: E0401: Unable to import 'pwd' (import-error) test\test_snapshots.py:24:0: E0401: Unable to import 'grp' (import-error) test\test_snapshots.py:42:13: E1101: Module 'os' has no 'geteuid' member (no-member) test\test_snapshots.py:45:13: E1101: Module 'os' has no 'getegid' member (no-member) test\test_snapshots.py:52:10: E1101: Module 'os' has no 'geteuid' member (no-member) ***** Module test.test_tools test\test_tools.py:33:0: E0401: Unable to import 'pyfakefs.fake_filesystem_unittest' (import-error) test\test_tools.py:248:33: E1101: Module 'signal' has no 'SIGSTOP' member (no-member) test\test_tools.py:251:33: E1101: Module 'signal' has no 'SIGCONT' member (no-member)

============================================================= short test summary info ============================================================= FAILED test/test_lint.py::MirrorMirrorOnTheWall::test_with_pylint - AssertionError: 0 != 54 : PyLint found 54 problems. =============================================================== 1 failed in 10.85s ======================================

buhtz commented 5 days ago

I believe I have installed ubuntu and the dependencies on my windows command prompt

This sentence confuses me. You installed ubuntu but using a windows command prompt? Are you using "Windows Subsystem for Linux" (WSL) instead of a real GNU Linux?

The import errors you reporting are very exotic and uncommon. Using a WSL might explain them. WSL is not supported. You can use Back In Time only on a real/full GNU Linux distribution but not on Windows.

If you still use a real GNU Linux then we need to investigate the problem further. Despite the unit tests are you able to start BIT?

I apologize for my lack of experience! However, I'm not sure what I am doing wrong.

No need to apologize. We are doing this to increase our experience and learn. :smile:

vincentfar commented 5 days ago

Oh I see, yeah I am currently on Windows working on VS code and for the terminal using cmd. I tried WSL because I figured that would work as a good substitute to GNU Linux but I guess not. Yeah not able to run BIT because it says "ModuleNotFoundError: No module named 'syslog'" Even though I already used: "pip install syslog-py" "Requirement already satisfied: syslog-py in c:\users\vince\anaconda3\lib\site-packages (0.2.5)".

I guess that means I can't work on this project on Windows?

buhtz commented 5 days ago

I guess that means I can't work on this project on Windows?

You can not provide new or modified code to the project, right. But there are several other ways to contribute. You might like to help translating the GUI with your native language? Or you can work on the documentation (user manual for example).

vincentfar commented 5 days ago

It's okay, thank you for all your help.

N0madC0de commented 4 days ago

Hello Christian,

I am 37. Started coding a few days ago after a gap of about 15 years. The last time I coded was in High School in C and it was a horrible experience. Always wanted to code but due to time constraints I was unable to do so. Now, I have decided to devote 2 hours daily. Presently learning python and it is so simple and powerful. I hope to contribute something to any Open Source project as I learn a bit more of Python.

Presently, I will read the CONTIRBUTION.md and try to follow the instructions.

Kind Regards, N0madC0de

buhtz commented 4 days ago

Hello N0madC0de, thank you for your message and your efforts. Did I get it correct that you are just start to learn Python?

Christian

N0madC0de commented 4 days ago

Started learning Python about 15 days ago.

I tried the test_lint.py. This is the output I got:

root@parb-B760M-DS3H-AX-DDR4:/home/parb/Documents/github-clones/backintime/common/test# pytest -v test_lint.py ====================================================================== test session starts ======================================================================= platform linux -- Python 3.12.3, pytest-7.4.4, pluggy-1.4.0 -- /usr/bin/python3 cachedir: .pytest_cache rootdir: /home/parb/Documents/github-clones/backintime/common/test collected 1 item

test_lint.py::MirrorMirrorOnTheWall::test_with_pylint FAILED [100%]

============================================================================ FAILURES ============================================================================ _ MirrorMirrorOnTheWall.test_withpylint

self = 

@unittest.skipUnless(ON_TRAVIS or PYLINT_AVIALBE, PYLINT_REASON)
def test_with_pylint(self):
    """Use Pylint to check for specific error codes.

    Some facts about PyLint
     - It is one of the slowest available linters.
     - It is able to catch lints none of the other linters
    """

    # Pylint base command
    cmd = [
        'pylint',
        # Make sure BIT modules can be imported (to detect "no-member")
        '--init-hook=import sys;'
        'sys.path.insert(0, "./../qt");'
        'sys.path.insert(0, "./../common");',
        # Storing results in a pickle file is unnecessary
        '--persistent=n',
        # autodetec number of parallel jobs
        '--jobs=0',
        # Disable scoring  ("Your code has been rated at xx/10")
        '--score=n',
        # Deactivate all checks by default
        '--disable=all',
        # prevent false-positive no-module-member errors
        '--extension-pkg-allow-list=PyQt6,PyQt6.QtCore',
        # Because of globally installed GNU gettext functions
        '--additional-builtins=_,ngettext',
        # PEP8 conform line length (see PyLint Issue #3078)
        '--max-line-length=79',
        # Whitelist variable names
        '--good-names=idx,fp',
        # '--reports=yes',
    ]

    # Explicit activate checks
    err_codes = [
        'C0305',  # trailing-newlines
        'C0324',  # superfluous-parens
        'C0410',  # multiple-imports
        'C0303',  # trailing-whitespace
        'E0100',  # init-is-generator
        'E0101',  # return-in-init
        'E0102',  # function-redefined
        'E0103',  # not-in-loop
        'E0106',  # return-arg-in-generator
        'E0213',  # no-self-argument
        'E0401',  # import-error
        'E0602',  # undefined-variable
        'E1101',  # no-member
        'W0311',  # bad-indentation
        'I0021',  # useless-suppression
        # 'W0611',  # unused-import
        'W1301',  # unused-format-string-key
        'W1401',  # anomalous-backslash-in-string (invalid escape sequence)

        # Enable asap. This list is selection of existing (not all!)
        # problems currently exiting in the BIT code base. Quit easy to fix
        # because there count is low.
        'R0201',  # no-self-use
        # 'R0202',  # no-classmethod-decorator
        # 'R0203',  # no-staticmethod-decorator
        # 'R0801',  # duplicate-code
        # 'W0123',  # eval-used
        # 'W0237',  # arguments-renamed
        # 'W0221',  # arguments-differ
        # 'W0404',  # reimported
        # 'W4902',  # deprecated-method
        # 'W4904',  # deprecated-class
        # 'W0603',  # global-statement
        # 'W0614',  # unused-wildcard-import
        # 'W0612',  # unused-variable
        # 'W0707',  # raise-missing-from
    ]

    cmd.append('--enable=' + ','.join(err_codes))

    # Add py files
    cmd.extend(self._collect_py_files())

    r = subprocess.run(
        cmd,
        check=False,
        universal_newlines=True,
        capture_output=True)

    # Count lines except module headings
    error_n = len(list(filter(lambda line: not line.startswith('*****'),
                              r.stdout.splitlines())))
    print(r.stdout)
  self.assertEqual(0, error_n, f'PyLint found {error_n} problems.')

E AssertionError: 0 != 8 : PyLint found 8 problems.

testlint.py:133: AssertionError ---------------------------------------------------------------------- Captured stdout call ---------------------------------------------------------------------- ***** Module tools /home/parb/Documents/github-clones/backintime/common/tools.py:56:8: E0401: Unable to import 'keyring' (import-error) /home/parb/Documents/github-clones/backintime/common/tools.py:57:8: E0401: Unable to import 'keyring' (import-error) /home/parb/Documents/github-clones/backintime/common/tools.py:58:8: E0401: Unable to import 'keyring.util.platform' (import-error) Module qt_probing /home/parb/Documents/github-clones/backintime/common/qt_probing.py:104:4: E0401: Unable to import 'PyQt6' (import-error) /home/parb/Documents/github-clones/backintime/common/qt_probing.py:105:4: E0401: Unable to import 'PyQt6.QtWidgets' (import-error) /home/parb/Documents/github-clones/backintime/common/qt_probing.py:120:4: E0401: Unable to import 'PyQt6.QtWidgets' (import-error) Module test.test_tools test_tools.py:33:0: E0401: Unable to import 'pyfakefs.fake_filesystem_unittest' (import-error) ***** Module test.test_config_crontab test_config_crontab.py:22:0: E0401: Unable to import 'pyfakefs.fake_filesystem_unittest' (import-error)

==================================================================== short test summary info ===================================================================== FAILED test_lint.py::MirrorMirrorOnTheWall::test_with_pylint - AssertionError: 0 != 8 : PyLint found 8 problems. ======================================================================= 1 failed in 3.06s ========================================================================

What to do next sir?

buhtz commented 3 days ago

Hello N0madC0de,

Started learning Python about 15 days ago.

I really appreciate your effort to contribute but I would say this is to early for us. We do accept that we need to invest some time and resource into guiding new contributors in the project. But this project is not a Python learning course. We can not effort that. We have no company and no money behind. We do this in our spare time beside our regular life. I am sorry.

About your output you provided. You shouldn't work with your root account. To dangerous. And the errors from the test run are about missing dependencies. You need to install them first as described in the dependencies section in the contribution info.

Maybe have a look at r/learnpython.

Best, Christian

N0madC0de commented 3 days ago

Thanks for the suggestions.