zeek / zeek-client

Experimental implementation of Zeek's future cluster management client
BSD 3-Clause "New" or "Revised" License
4 stars 4 forks source link

Address a few pylint lint categories #33

Closed bbannier closed 1 year ago

ckreibich commented 1 year ago

Thanks for doing all of this, Benjamin.

I sort of struggle with reviewing these kinds of PRs. I'd find it easier if I could see the actual complaints in the PR without having to replicate them locally. Perhaps the easiest thing would be to simply attach the whole report of warnings to the PR? Suggestions welcome.

Also — not sure about your takes, but I don't walk away from this one convinced that pylint is really worth our time.

bbannier commented 1 year ago

Also — not sure about your takes, but I don't walk away from this one convinced that pylint is really worth our time.

I agree, making these cleanups is tedious and most of the changes are pretty underwhelming. I believe they do however sometimes point to deeper issues (which in my superficial fixes I didn't address). I believe it is perfectly possible to write pylint-conformant code with only very localized use of suppressions, so my goal was to bring this code base to a point where we get warnings from pylint by default during development (as opposed to suppressing all non-fatal warnings with the pylint -E removed in #32); this should then hopefully organically lead to better style and maybe design, e.g., applying the changes from #31 on top of this branch would lead to useful suggestions.

I sort of struggle with reviewing these kinds of PRs. I'd find it easier if I could see the actual complaints in the PR without having to replicate them locally. Perhaps the easiest thing would be to simply attach the whole report of warnings to the PR? Suggestions welcome.

I am not sure either how to best do that, so I'll just paste the warnings explicitly suppressed in #32:

************* Module setup
setup.py:1:0: C0114: Missing module docstring (missing-module-docstring)
setup.py:5:12: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
setup.py:5:12: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
setup.py:7:21: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
setup.py:7:21: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
************* Module test_brokertypes
tests/test_brokertypes.py:14:0: W0401: Wildcard import zeekclient.brokertypes (wildcard-import)
tests/test_brokertypes.py:14:0: C0413: Import "from zeekclient.brokertypes import *" should be placed at the top of the module (wrong-import-position)
tests/test_brokertypes.py:18:4: C0103: Method name "assertEqualRoundtrip" doesn't conform to snake_case naming style (invalid-name)
tests/test_brokertypes.py:18:35: W0622: Redefining built-in 'input' (redefined-builtin)
tests/test_brokertypes.py:24:4: C0103: Method name "assertHash" doesn't conform to snake_case naming style (invalid-name)
tests/test_brokertypes.py:485:12: W0106: Expression "Boolean(True) < True" is assigned to nothing (expression-not-assigned)
tests/test_brokertypes.py:487:12: W0106: Expression "Boolean(True) < HandshakeMessage" is assigned to nothing (expression-not-assigned)
tests/test_brokertypes.py:495:12: W0612: Unused variable 'obj' (unused-variable)
tests/test_brokertypes.py:502:12: W0612: Unused variable 'obj' (unused-variable)
tests/test_brokertypes.py:17:0: R0904: Too many public methods (27/20) (too-many-public-methods)
tests/test_brokertypes.py:14:0: W0614: Unused import(s) abc, enum, json, re, Type, DataType, MessageType and from_broker from wildcard import of zeekclient.brokertypes (unused-wildcard-import)
************* Module test_cli
tests/test_cli.py:23:0: C0413: Import "import zeekclient as zc" should be placed at the top of the module (wrong-import-position)
tests/test_cli.py:96:20: W1510: 'subprocess.run' used without explicitly defining the value for 'check'. (subprocess-run-check)
tests/test_cli.py:134:27: W0613: Unused argument 'prefix' (unused-argument)
tests/test_cli.py:154:4: C0103: Method name "assertLogLines" doesn't conform to snake_case naming style (invalid-name)
tests/test_cli.py:176:8: R1711: Useless return at end of function or method (useless-return)
tests/test_cli.py:116:0: R0904: Too many public methods (32/20) (too-many-public-methods)
************* Module test_config_io
tests/test_config_io.py:345:0: C0301: Line too long (111/100) (line-too-long)
tests/test_config_io.py:21:0: C0413: Import "import zeekclient" should be placed at the top of the module (wrong-import-position)
tests/test_config_io.py:140:4: C0103: Method name "assertEqualStripped" doesn't conform to snake_case naming style (invalid-name)
tests/test_config_io.py:143:4: C0103: Method name "parserFromString" doesn't conform to snake_case naming style (invalid-name)
tests/test_config_io.py:194:16: C3001: Lambda expression assigned to a variable. Define a function using the "def" keyword instead. (unnecessary-lambda-assignment)
************* Module test_config_overrides
tests/test_config_overrides.py:17:0: C0413: Import "import zeekclient" should be placed at the top of the module (wrong-import-position)
************* Module test_controller
tests/test_controller.py:135:79: W1401: Anomalous backslash in string: '\('. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:135:81: W1401: Anomalous backslash in string: '\)'. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:149:79: W1401: Anomalous backslash in string: '\('. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:149:81: W1401: Anomalous backslash in string: '\)'. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:158:46: W1401: Anomalous backslash in string: '\('. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:158:48: W1401: Anomalous backslash in string: '\)'. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:169:47: W1401: Anomalous backslash in string: '\('. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:169:49: W1401: Anomalous backslash in string: '\)'. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:178:43: W1401: Anomalous backslash in string: '\('. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:178:45: W1401: Anomalous backslash in string: '\)'. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:187:47: W1401: Anomalous backslash in string: '\('. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:187:49: W1401: Anomalous backslash in string: '\)'. String constant might be missing an r prefix. (anomalous-backslash-in-string)
tests/test_controller.py:22:0: C0413: Import "import websocket" should be placed at the top of the module (wrong-import-position)
tests/test_controller.py:24:0: C0413: Import "import zeekclient" should be placed at the top of the module (wrong-import-position)
tests/test_controller.py:35:4: C0103: Method name "assertLogLines" doesn't conform to snake_case naming style (invalid-name)
tests/test_controller.py:124:16: W0612: Unused variable 'controller' (unused-variable)
tests/test_controller.py:27:0: R0904: Too many public methods (26/20) (too-many-public-methods)
tests/test_controller.py:9:0: W0611: Unused patch imported from unittest.mock (unused-import)
************* Module test_types
tests/test_types.py:13:0: C0413: Import "import zeekclient.brokertypes as bt" should be placed at the top of the module (wrong-import-position)
tests/test_types.py:14:0: W0401: Wildcard import zeekclient.types (wildcard-import)
tests/test_types.py:14:0: C0413: Import "from zeekclient.types import *" should be placed at the top of the module (wrong-import-position)
tests/test_types.py:18:4: C0103: Method name "assertHash" doesn't conform to snake_case naming style (invalid-name)
tests/test_types.py:22:35: W0622: Redefining built-in 'input' (redefined-builtin)
tests/test_types.py:32:12: W0104: Statement seems to have no effect (pointless-statement)
tests/test_types.py:121:8: W0612: Unused variable 'val1' (unused-variable)
tests/test_types.py:14:0: W0614: Unused import(s) configparser, enum, shlex, socket, ConfigParserMixin, SerializableZeekType, JsonableZeekType, ZeekType, Enum, make_uuid and LOG from wildcard import of zeekclient.types (unused-wildcard-import)
************* Module websocket
tests/websocket.py:22:0: W0613: Unused argument 'args' (unused-argument)
tests/websocket.py:22:0: W0613: Unused argument 'kwargs' (unused-argument)
tests/websocket.py:48:0: W0613: Unused argument 'options' (unused-argument)
************* Module zeek-client
zeek-client:1:0: C0103: Module name "zeek-client" doesn't conform to snake_case naming style (invalid-name)
************* Module zeekclient.brokertypes
zeekclient/brokertypes.py:92:0: R0022: Useless option value for 'disable', 'no-self-use' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)
zeekclient/brokertypes.py:104:0: R0022: Useless option value for 'disable', 'no-self-use' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)
zeekclient/brokertypes.py:852:9: W0511: TODO: Extend to handle metadata (fixme)
zeekclient/brokertypes.py:42:11: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/brokertypes.py:46:8: C0206: Consider iterating with .items() (consider-using-dict-items)
zeekclient/brokertypes.py:154:11: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/brokertypes.py:334:11: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/brokertypes.py:361:4: R0911: Too many return statements (7/6) (too-many-return-statements)
zeekclient/brokertypes.py:394:8: W0622: Redefining built-in 'format' (redefined-builtin)
zeekclient/brokertypes.py:438:11: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/brokertypes.py:854:32: W0212: Access to a protected member _elements of a client class (protected-access)
zeekclient/brokertypes.py:897:0: W1404: Implicit string concatenation found in call (implicit-str-concat)
zeekclient/brokertypes.py:921:0: W1404: Implicit string concatenation found in call (implicit-str-concat)
zeekclient/brokertypes.py:1179:12: W0212: Access to a protected member _elements of a client class (protected-access)
zeekclient/brokertypes.py:1185:12: W0212: Access to a protected member _elements of a client class (protected-access)
zeekclient/brokertypes.py:1191:12: W0212: Access to a protected member _elements of a client class (protected-access)
************* Module zeekclient.cli
zeekclient/cli.py:112:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:281:21: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:281:15: W0613: Unused argument 'args' (unused-argument)
zeekclient/cli.py:354:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:379:9: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
zeekclient/cli.py:392:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:462:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:500:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:573:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:589:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:669:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:715:20: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
zeekclient/cli.py:731:4: W0621: Redefining name 'controller' from outer scope (line 11) (redefined-outer-name)
************* Module zeekclient.config
zeekclient/config.py:113:0: W1404: Implicit string concatenation found in call (implicit-str-concat)
************* Module zeekclient.consts
zeekclient/consts.py:1:0: C0114: Missing module docstring (missing-module-docstring)
************* Module zeekclient.controller
zeekclient/controller.py:177:23: W0718: Catching too general exception Exception (broad-exception-caught)
zeekclient/controller.py:144:16: W0612: Unused variable 'err' (unused-variable)
zeekclient/controller.py:293:23: W0718: Catching too general exception Exception (broad-exception-caught)
zeekclient/controller.py:289:16: W0612: Unused variable 'err' (unused-variable)
zeekclient/controller.py:23:0: W0611: Unused ErrorMessage imported from brokertypes (unused-import)
zeekclient/controller.py:23:0: W0611: Unused unserialize imported from brokertypes (unused-import)
************* Module zeekclient.types
zeekclient/types.py:25:0: R0022: Useless option value for 'disable', 'no-self-use' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)
zeekclient/types.py:60:0: R0022: Useless option value for 'disable', 'no-self-use' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)
zeekclient/types.py:244:32: W0511: XXX needs proper optionality (fixme)
zeekclient/types.py:75:0: R0903: Too few public methods (1/2) (too-few-public-methods)
zeekclient/types.py:102:11: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/types.py:108:12: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/types.py:157:20: W0612: Unused variable 'name' (unused-variable)
zeekclient/types.py:222:12: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/types.py:254:12: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/types.py:325:12: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/types.py:426:4: R0914: Too many local variables (21/15) (too-many-locals)
zeekclient/types.py:602:12: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/types.py:764:12: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/types.py:854:12: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
zeekclient/types.py:925:12: C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)
************* Module zeekclient.utils
zeekclient/utils.py:1:0: C0114: Missing module docstring (missing-module-docstring)
zeekclient/utils.py:1:0: R0801: Similar lines in 2 files
==test_cli:[154:167]
==test_controller:[35:48]
        buflines = self.logbuf.getvalue().split("\n")
        todo = list(patterns)
        for line in buflines:
            if todo and re.search(todo[0], line) is not None:
                todo.pop(0)
        msg = None
        if todo:
            msg = "log pattern '{}' not found; have:\n{}".format(
                todo[0], self.logbuf.getvalue().strip()
            )
        self.assertEqual(len(todo), 0, msg)

    def test_connect_successful(self): (duplicate-code)
zeekclient/utils.py:1:0: R0401: Cyclic import (websocket -> zeekclient -> zeekclient.cli -> zeekclient.controller) (cyclic-import)
zeekclient/utils.py:1:0: R0401: Cyclic import (websocket -> zeekclient -> zeekclient.controller) (cyclic-import)
ckreibich commented 1 year ago

Yeah, likewise agree. It's also completely true that many of my constructs for testing weren't the proper Python way of doing things, and no doubt the code is in better shape now. So thanks again!