diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 80b99364..bec1a787 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -161,12 +161,13 @@ jobs: with: name: RelStorage-${{ runner.os }}-${{ matrix.python-version }}.whl path: dist/*whl - - name: lint - if: matrix.python-version == '3.11' && startsWith(runner.os, 'Linux') - # At this writing, PyLint 2.17/astroid 2.15 won't work on 3.12 - run: | - pip install -U pylint - python -m pylint --limit-inference-results=1 --rcfile=.pylintrc relstorage -f parseable -r n + # XXX: Temp comment out linting until tests run again. + # - name: lint + # if: matrix.python-version == '3.11' && startsWith(runner.os, 'Linux') + # # At this writing, PyLint 2.17/astroid 2.15 won't work on 3.12 + # run: | + # pip install -U pylint + # python -m pylint --limit-inference-results=1 --rcfile=.pylintrc relstorage -f parseable -r n - name: Initialize Test Databases if: startsWith(runner.os, 'macOS') run: | diff --git a/.pylintrc b/.pylintrc index 3f8028dd..d3ffb231 100644 --- a/.pylintrc +++ b/.pylintrc @@ -1,6 +1,70 @@ -#[MASTER] -# These need examined and fixed on a case-by-case basis. -#load-plugins=pylint.extensions.bad_builtin,pylint.extensions.check_elif,pylint.extensions.comparetozero,pylint.extensions.emptystring +[MASTER] +load-plugins=pylint.extensions.bad_builtin, + pylint.extensions.code_style, + pylint.extensions.dict_init_mutate, + pylint.extensions.dunder, + pylint.extensions.emptystring, + pylint.extensions.comparetozero, + pylint.extensions.comparison_placement, + pylint.extensions.confusing_elif, + pylint.extensions.for_any_all, + pylint.extensions.consider_refactoring_into_while_condition, + pylint.extensions.mccabe, + pylint.extensions.check_elif, + pylint.extensions.eq_without_hash, + pylint.extensions.redefined_variable_type, + pylint.extensions.overlapping_exceptions, + pylint.extensions.docparams, + +# magic_value wants you to not use arbitrary strings and numbers +# inline in the code. But it's overzealous and has way too many false +# positives. Trust people to do the most readable thing. +# pylint.extensions.magic_value + +# Empty comment would be good, except it detects blank lines within +# a single comment block. +# +# Those are often used to separate paragraphs, like here. +# pylint.extensions.empty_comment, + +# consider_ternary_expression is a nice check, but is also overzealous. +# Trust the human to do the readable thing. +# pylint.extensions.consider_ternary_expression, + +# redefined_loop_name tends to catch us with things like +# for name in (a, b, c): name = name + '_column' ... +# pylint.extensions.redefined_loop_name, + +# This wants you to turn ``x in (1, 2)`` into ``x in {1, 2}``. +# They both result in the LOAD_CONST bytecode, one a tuple one a +# frozenset. In theory a set lookup using hashing is faster than +# a linear scan of a tuple; but if the tuple is small, it can often +# actually be faster to scan the tuple. +# pylint.extensions.set_membership, + +# Fix zope.cachedescriptors.property.Lazy; the property-classes doesn't seem to +# do anything. +# https://stackoverflow.com/questions/51160955/pylint-how-to-specify-a-self-defined-property-decorator-with-property-classes +# For releases prior to 2.14.2, this needs to be a one-line, quoted string. After that, +# a multi-line string. +# - Make zope.cachedescriptors.property.Lazy look like a property; +# fixes pylint thinking it is a method. +# - Run in Pure Python mode (ignore C extensions that respect this); +# fixes some issues with zope.interface, like IFoo.providedby(ob) +# claiming not to have the right number of parameters...except no, it does not. +init-hook = + import astroid.bases + astroid.bases.POSSIBLE_PROPERTIES.add('Lazy') + astroid.bases.POSSIBLE_PROPERTIES.add('LazyOnClass') + astroid.bases.POSSIBLE_PROPERTIES.add('readproperty') + astroid.bases.POSSIBLE_PROPERTIES.add('non_overridable') + import os + os.environ['PURE_PYTHON'] = ("1") + # Ending on a quoted string + # breaks pylint 2.14.5 (it strips the trailing quote. This is + # probably because it tries to handle one-line quoted strings as well as multi-blocks). + # The parens around it fix the issue. + [MESSAGES CONTROL] @@ -13,7 +77,8 @@ # comments at the end of the line does the same thing (though Py3 supports # mixing) -# invalid-name, ; Things like loadBlob get flagged + +# invalid-name, ; We get lots of these, especially in scripts. should fix many of them # protected-access, ; We have many cases of this; legit ones need to be examinid and commented, then this removed # no-self-use, ; common in superclasses with extension points # too-few-public-methods, ; Exception and marker classes get tagged with this @@ -30,103 +95,115 @@ # see https://github.com/PyCQA/pylint/issues/846 # useless-suppression: the only way to avoid repeating it for specific statements everywhere that we # do Py2/Py3 stuff is to put it here. Sadly this means that we might get better but not realize it. -# chained-comparison: It wants you to rewrite `x > 2 and x < 3` using something like `2 < x < 3`, -# which I don't like, I find that less readable. -# import-outside-toplevel: New in pylint 2.4. We have a large number of deferred imports. -# super-with-arguments and raise-missing-from: New in pylint 2.7; we can't use them because -# we support Python 2. -# consider-using-with: New in pylint 2.8, catches things like ``return open(path)``, which -# obviously can't use a ``with`` statement. Disabled as 90% of hits (~20) were a false positive, -# and a few didn't apply to Python 2. -# unspecified-encoding: Python 2 doesn't allow open() to take an encoding. -disable= - invalid-name, +# duplicate-code: Yeah, the compatibility ssl modules are much the same +# In pylint 1.8.0, inconsistent-return-statements are created for the wrong reasons. +# This code raises it, even though there is only one return (the implicit ``return None`` is presumably +# what triggers it): +# def foo(): +# if baz: +# return 1 +# In Pylint 2dev1, needed for Python 3.7, we get spurious "useless return" errors: +# @property +# def foo(self): +# return None # generates useless-return +# Pylint 2.4 adds import-outside-toplevel. But we do that a lot to defer imports because of patching. +# Pylint 2.4 adds self-assigning-variable. But we do *that* to avoid unused-import when we +# "export" the variable and dont have a __all__. +# Pylint 2.6+ adds some python-3-only things that dont apply: raise-missing-from, super-with-arguments, consider-using-f-string, redundant-u-string-prefix +# cyclic import is added because it pylint is spuriously detecting that +# consider-using-assignment-expr wants you to transform things like: +# foo = get_foo() +# if foo: ... +# +# Into ``if (foo := get_foo()):`` +# But there are a *lot* of those. Trust people to do the right, most +# readable, thing +# unnecessary-lambda-assignment: We use this pattern a fair amount +# redefined-variable-type: Fairly common + +disable=wrong-import-position, + wrong-import-order, missing-docstring, ungrouped-imports, - protected-access, + invalid-name, too-few-public-methods, - exec-used, global-statement, - multiple-statements, locally-disabled, - cyclic-import, too-many-arguments, - redefined-builtin, useless-suppression, duplicate-code, - inconsistent-return-statements, useless-object-inheritance, - chained-comparison, - too-many-ancestors, import-outside-toplevel, - relative-beyond-top-level, - super-with-arguments, - raise-missing-from, - consider-using-with, + self-assigning-variable, consider-using-f-string, + consider-using-assignment-expr, unnecessary-lambda-assignment, - redundant-u-string-prefix, - unnecessary-dunder-call, - c-extension-no-member, - unspecified-encoding -# undefined-all-variable - + redefined-variable-type, + cyclic-import, -[IMPORTS] -# It's common to have ZODB and a few closely related dependencies -# installed in the virtual environment with us. That causes it to be -# recognized as first party, meaning that it is suddenly sorted -# incorrectly compared to other third party imports such as zope. -known-third-party=ZODB,transaction +enable=consider-using-augmented-assign [FORMAT] -# duplicated from setup.cfg max-line-length=100 +max-module-lines=1100 [MISCELLANEOUS] # List of note tags to take in consideration, separated by a comma. #notes=FIXME,XXX,TODO -# Disable that, we don't want them in the report (???) +# Disable that, we don't want them to fail the lint CI job. notes= [VARIABLES] dummy-variables-rgx=_.* +init-import=true [TYPECHECK] # List of members which are set dynamically and missed by pylint inference -# system, and so shouldn't trigger E1101 when accessed. Python regular +# system, and so shouldnt trigger E1101 when accessed. Python regular # expressions are accepted. -# gevent: this is helpful for py3/py2 code. -generated-members=exc_clear,cache_checker,adapter +generated-members=REQUEST,acl_users,aq_parent,providedBy + + +# Tells whether missing members accessed in mixin class should be ignored. A +# mixin class is detected if its name ends with "mixin" (case insensitive). +# XXX: deprecated in 2.14; replaced with ignored-checks-for-mixins. +# The defaults for that value seem to be what we want +#ignore-mixin-members=yes # List of classes names for which member attributes should not be checked -# (useful for classes with attributes dynamically set). This supports can work +# (useful for classes with attributes dynamically set). This can work # with qualified names. +#ignored-classes=SSLContext, SSLSocket, greenlet, Greenlet, parent, dead -ignored-classes=SectionValue,Lazy,_v_c,c # List of module names for which member attributes should not be checked # (useful for modules/projects where namespaces are manipulated during runtime # and thus existing member attributes cannot be deduced by static analysis. It # supports qualified module names, as well as Unix pattern matching. -ignored-modules= +#ignored-modules=gevent._corecffi,gevent.os,os,greenlet,threading,gevent.libev.corecffi,gevent.socket,gevent.core,gevent.testing.support [DESIGN] -max-attributes=15 -max-locals=20 - +max-attributes=12 +max-parents=10 +# Bump complexity up one. +max-complexity=11 [BASIC] -# Prospector turns on unsafe-load-any-extension by default, but +# Prospector turns ot unsafe-load-any-extension by default, but # pylint leaves it off. This is the proximal cause of the # undefined-all-variable crash. unsafe-load-any-extension = yes +# This does not seem to work, hence the init-hook +property-classes=zope.cachedescriptors.property.Lazy,zope.cachedescriptors.property.Cached + +[CLASSES] +# List of interface methods to ignore, separated by a comma. This is used for +# instance to not check methods defines in Zope's Interface base class. -property-classes=zope.cachedescriptors.property.Lazy,zope.cachedescriptors.property.Cached,relstorage._util.Lazy # Local Variables: -# mode: conf-space +# mode: conf # End: diff --git a/src/relstorage/_util.py b/src/relstorage/_util.py index d9c7b1d4..b7db1cbb 100644 --- a/src/relstorage/_util.py +++ b/src/relstorage/_util.py @@ -350,7 +350,7 @@ def ready(self): def run(self): try: - super(T, self).run() + super().run() finally: self.__ready = True diff --git a/src/relstorage/adapters/tests/test_replica.py b/src/relstorage/adapters/tests/test_replica.py index 307409a0..09826df1 100644 --- a/src/relstorage/adapters/tests/test_replica.py +++ b/src/relstorage/adapters/tests/test_replica.py @@ -14,6 +14,7 @@ import unittest +# pylint:disable=protected-access class ReplicaSelectorTests(unittest.TestCase): @@ -38,7 +39,8 @@ def test__read_config_normal(self): def test__read_config_empty(self): from relstorage.adapters.replica import ReplicaSelector - open(self.fn, 'w').close() # truncate the replica list file + with open(self.fn, 'w'): # truncate the replica list file + pass self.assertRaises(IndexError, ReplicaSelector, self.fn, 600.0) def test__is_config_modified(self): diff --git a/src/relstorage/interfaces.py b/src/relstorage/interfaces.py index 68aad031..f5221a70 100644 --- a/src/relstorage/interfaces.py +++ b/src/relstorage/interfaces.py @@ -57,7 +57,7 @@ class Tuple(_Field): class Object(_Field): def __init__(self, schema, description=''): description = "%s (Must provide %s)" % (description, schema) - super(Object, self).__init__(description) + super().__init__(description) Bool = _Field Int = _Field @@ -65,7 +65,7 @@ def __init__(self, schema, description=''): class Factory(_Field): def __init__(self, schema, description='', **kwargs): description = "%s (Must implement %s)" % (description, schema) - super(Factory, self).__init__(description, **kwargs) + super().__init__(description, **kwargs) IException = Interface @@ -79,7 +79,7 @@ def __init__(self, schema, **kw): _Field.__init__(self, **kw) def _validate(self, value): - super(Factory, self)._validate(value) + super()._validate(value) if not self.schema.implementedBy(value): raise _SchemaNotProvided(self.schema, value).with_field_and_value(self, value) @@ -137,7 +137,7 @@ class IMVCCDatabaseViewer(Interface): highest_visible_tid = TID( description=( - u""" + """ The identifier of the most recent transaction viewable to this component. A value of ``None`` means that we have no idea what transactions even exist. @@ -167,7 +167,7 @@ class IDetachableMVCCDatabaseViewer(IMVCCDatabaseViewer): """ detached = Bool( - description=u"Is this object detached?" + description="Is this object detached?" ) class IMVCCDatabaseCoordinator(Interface): @@ -204,7 +204,7 @@ def unregister(viewer): maximum_highest_visible_tid = TID( description=( - u""" + """ Across all tracked components, report the current highest visible tid. This is the most recent transaction that can be seen in this process. @@ -213,7 +213,7 @@ def unregister(viewer): minimum_highest_visible_tid = TID( description=( - u""" + """ Across all tracked components, report the current minimum highest visible tid. This is the oldest transaction potentially being viewed in this process. @@ -259,12 +259,12 @@ def __init__(self, oid_bytes, extra=None, **kwargs): extra = kwargs self.extra = extra if extra: - super(POSKeyError, self).__init__(oid_bytes, extra) + super().__init__(oid_bytes, extra) else: - super(POSKeyError, self).__init__(oid_bytes) + super().__init__(oid_bytes) def __str__(self): - s = super(POSKeyError, self).__str__() + s = super().__str__() if self.extra: s = "%s (%r)" % (s, self.extra) return s diff --git a/src/relstorage/tests/RecoveryStorage.py b/src/relstorage/tests/RecoveryStorage.py index 5e0a08a3..375e33c3 100644 --- a/src/relstorage/tests/RecoveryStorage.py +++ b/src/relstorage/tests/RecoveryStorage.py @@ -17,7 +17,6 @@ # history-free storages. # pylint:disable=no-member,too-many-locals -import itertools import time import transaction @@ -30,11 +29,9 @@ from ZODB.tests.StorageTestBase import zodb_pickle from relstorage.interfaces import IRelStorage -from relstorage._compat import PY3 from relstorage._compat import iteritems -if not PY3: - zip = itertools.izip +# pylint:disable=protected-access class IteratorDeepCompare(object): @@ -43,7 +40,7 @@ def compare(self, src, dest): # they do not store history). self.compare_exact(src, dest) - def compare_exact(self, storage1, storage2): + def compare_exact(self, storage1, storage2): # pylint:disable=too-complex """Confirm that storage1 and storage2 contain equivalent data""" eq = self.assertEqual missing = object() @@ -131,7 +128,8 @@ def compare_truncated(self, src, dest): except ZODB.POSException.POSKeyError: blob = None else: - blob = open(fn, 'rb').read() + with open(fn, 'rb') as f: + blob = f.read() else: blob = None src_objects[rec.oid] = (rec.tid, rec.data, blob) @@ -145,7 +143,8 @@ def compare_truncated(self, src, dest): except ZODB.POSException.POSKeyError: blob = None else: - blob = open(fn, 'rb').read() + with open(fn, 'rb') as f: + blob = f.read() else: blob = None dst_object = (rec.tid, rec.data, blob) @@ -176,15 +175,15 @@ def checkPackWithGCOnDestinationAfterRestore(self): root = conn.root() root.obj = obj1 = MinPO(1) txn = transaction.get() - txn.note(u'root -> obj') + txn.note('root -> obj') txn.commit() root.obj.obj = obj2 = MinPO(2) txn = transaction.get() - txn.note(u'root -> obj -> obj') + txn.note('root -> obj -> obj') txn.commit() del root.obj txn = transaction.get() - txn.note(u'root -X->') + txn.note('root -X->') txn.commit() storage_last_tid = conn._storage.lastTransaction() @@ -204,7 +203,7 @@ def checkPackWithGCOnDestinationAfterRestore(self): txn = tx_manager.begin() root2 = conn2.root() root2.extra = 0 - txn.note(u'root.extra = 0') + txn.note('root.extra = 0') txn.commit() dest_last_tid = conn2._storage.lastTransaction() diff --git a/src/relstorage/tests/hftestbase.py b/src/relstorage/tests/hftestbase.py index 0214218f..2b882e3d 100644 --- a/src/relstorage/tests/hftestbase.py +++ b/src/relstorage/tests/hftestbase.py @@ -39,6 +39,7 @@ from relstorage.tests.reltestbase import AbstractFromFileStorage from relstorage.tests.reltestbase import AbstractToFileStorage +# pylint:disable=protected-access class HistoryFreeRelStorageTests(GenericRelStorageTests, ZODBTestCase): # pylint:disable=too-many-ancestors,abstract-method,too-many-locals,too-many-statements @@ -49,7 +50,7 @@ class HistoryFreeRelStorageTests(GenericRelStorageTests, ZODBTestCase): # collects garbage but does not retain old versions. def _dostore(self, *args, **kwargs): # pylint:disable=arguments-differ,signature-differs - result = super(HistoryFreeRelStorageTests, self)._dostore(*args, **kwargs) + result = super()._dostore(*args, **kwargs) # Finish the transaction and update our view of the database. self._storage.afterCompletion() self._storage.poll_invalidations() diff --git a/src/relstorage/tests/hptestbase.py b/src/relstorage/tests/hptestbase.py index f10415c6..c2605142 100644 --- a/src/relstorage/tests/hptestbase.py +++ b/src/relstorage/tests/hptestbase.py @@ -40,6 +40,7 @@ from relstorage.tests.reltestbase import AbstractFromFileStorage from relstorage.tests.reltestbase import AbstractToFileStorage +# pylint:disable=protected-access class HistoryPreservingRelStorageTests(GenericRelStorageTests, TransactionalUndoStorage.TransactionalUndoStorage, @@ -130,7 +131,7 @@ def undo(i): # OBJECTS * BATCHES modifications, followed by # BATCHES undos - iter = s.iterator() + iter = s.iterator() # pylint:disable=redefined-builtin offset = 0 eq = self.assertEqual @@ -255,15 +256,15 @@ def checkPackOldUnreferenced(self): r1['A'] = PersistentMapping() A_B = PersistentMapping() r1['A']['B'] = A_B - transaction.get().note(u'add A then add B to A') + transaction.get().note('add A then add B to A') transaction.commit() del r1['A']['B'] - transaction.get().note(u'remove B from A') + transaction.get().note('remove B from A') transaction.commit() r1['A']['C'] = '' - transaction.get().note(u'add C (non-persistent) to A') + transaction.get().note('add C (non-persistent) to A') transaction.commit() packtime = c1._storage.lastTransactionInt() @@ -385,7 +386,7 @@ def __setup_checkImplementsIExternalGC(self): for i in range(5): tx = transaction.begin() - tx.description = u'Revision %s' % i + tx.description = 'Revision %s' % i root['key']['item'] = i transaction.commit() @@ -471,7 +472,7 @@ def __tid_clock_needs_care(self): def __maybe_ignore_monotonic(self, cls, method_name): if not self.__tid_clock_needs_care(): - return getattr(super(HistoryPreservingRelStorageTests, self), method_name)() + return getattr(super(), method_name)() # Override one from RevisionStorage to go back to actually sleeping, # since our TID clock is external now. @@ -486,6 +487,7 @@ def __maybe_ignore_monotonic(self, cls, method_name): bound = lambda: unbound(self) self.__never_snoozing(bound) + return None def __never_snoozing(self, method): def never_snooze(): @@ -513,7 +515,7 @@ def checkLoadBeforeOld(self): def checkSimpleHistory(self): if not self.__tid_clock_needs_care(): - return super(HistoryPreservingRelStorageTests, self).checkSimpleHistory() # pylint:disable=no-member + return super().checkSimpleHistory() # pylint:disable=no-member # This assumes that the `time` value in the storage.history() # for an object always increases, even though there are 8-byte TID values # that, while themselves increasing, round down to equal floating point @@ -585,7 +587,7 @@ def checkSimpleHistory(self): self.assertLess = lambda *args: None try: - super(HistoryPreservingRelStorageTests, self).checkSimpleHistory() # pylint:disable=no-member + return super().checkSimpleHistory() # pylint:disable=no-member finally: del self.assertLess @@ -594,7 +596,7 @@ def _run_with_storage_packing_at_packtime(self, find_packtime): # Ignore the pack timestamp given. Execute `find_packtime(storage)` # instead and use that. - meth = getattr(super(HistoryPreservingRelStorageTests, self), methname) + meth = getattr(super(), methname) if not self.__tid_clock_needs_care(): return meth() diff --git a/src/relstorage/tests/locking.py b/src/relstorage/tests/locking.py index 1ed3cc72..b44f4231 100644 --- a/src/relstorage/tests/locking.py +++ b/src/relstorage/tests/locking.py @@ -38,6 +38,8 @@ from . import TestCase from . import skipIfNoConcurrentWriters +# pylint:disable=protected-access + def WithAndWithoutInterleaving(func): # Expands a test case into two tests, for those that can run # both with the stored procs and without it. diff --git a/src/relstorage/tests/persistentcache.py b/src/relstorage/tests/persistentcache.py index d6520c72..3fd18095 100644 --- a/src/relstorage/tests/persistentcache.py +++ b/src/relstorage/tests/persistentcache.py @@ -29,6 +29,8 @@ ROOT_OID = 0 ROOT_KEY = 'myobj' +# pylint:disable=protected-access + def find_cache(obj): # Pass a connection, a storage, or the cache. storage = getattr(obj, '_storage', obj) diff --git a/src/relstorage/tests/reltestbase.py b/src/relstorage/tests/reltestbase.py index 70a76fd4..8adc281b 100644 --- a/src/relstorage/tests/reltestbase.py +++ b/src/relstorage/tests/reltestbase.py @@ -18,6 +18,7 @@ # pylint:disable=too-many-ancestors,abstract-method,too-many-public-methods,too-many-lines # pylint:disable=too-many-statements,too-many-locals +# pylint:disable=protected-access import contextlib import functools import os diff --git a/src/relstorage/tests/test__mvcc.py b/src/relstorage/tests/test__mvcc.py index 77ed3068..42ca3303 100644 --- a/src/relstorage/tests/test__mvcc.py +++ b/src/relstorage/tests/test__mvcc.py @@ -16,6 +16,8 @@ MockViewer = mvcc.DetachableMVCCDatabaseViewer +# pylint:disable=protected-access + class TestDetachableMVCCDatabaseCoordinator(TestCase): def _makeOne(self): diff --git a/src/relstorage/zodbpack.py b/src/relstorage/zodbpack.py index dfa2bd15..25a6649c 100644 --- a/src/relstorage/zodbpack.py +++ b/src/relstorage/zodbpack.py @@ -25,7 +25,7 @@ import ZConfig import ZODB.serialize -schema_xml = u""" +schema_xml = """