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

Fix deprecation warnings #38

Merged
merged 18 commits into from
Aug 10, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ ignore =
bootstrap.py

[flake8]
ignore = C901,N801,N802,N803,N805,N806,N812,E301
ignore = C901,N801,N802,N803,N805,N806,N812,E301,P001
Copy link
Member

Choose a reason for hiding this comment

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

flake8-plone-api should not be used on this repos, so ignoring P001 should not be necessary.

exclude = bootstrap.py

[bdist_wheel]
Expand Down
14 changes: 7 additions & 7 deletions src/Products/PluginIndexes/BooleanIndex/BooleanIndex.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class BooleanIndex(UnIndex):
has a roughly equal 50/50 split.
"""

meta_type = "BooleanIndex"
meta_type = 'BooleanIndex'

manage_options = (
{'label': 'Settings',
Expand All @@ -55,7 +55,7 @@ class BooleanIndex(UnIndex):
'action': 'manage_browse'},
)

query_options = ["query"]
query_options = ['query']

manage = manage_main = DTMLFile('dtml/manageBooleanIndex', globals())
manage_main._setName('manage_main')
Expand Down Expand Up @@ -137,8 +137,8 @@ def removeForwardIndexEntry(self, entry, documentId, check=True):
raise
except Exception:
LOG.exception(
'%s: unindex_object could not remove documentId %s '
'from index %s. This should not happen.' % (
'{0}: unindex_object could not remove documentId {1} '
'from index {2}. This should not happen.'.format(
Copy link
Member

Choose a reason for hiding this comment

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

Although on other places it is beneficial to change to the .format() version, for logging it might be better to add the parameter as *args to the LOG.exception() as it gets formatted later on anyway. But as the initial case did not used this way, I can live with that here.

Copy link
Member

Choose a reason for hiding this comment

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

I'd really like to see using the logging parameters in the intended way: https://docs.python.org/3/howto/logging.html#logging-variable-data Logging does not (yet?) support format. The reason behind the % notation in the format string and the added values using commas is a performance one: The logging text is only rendered when it is actually logged. To me it seems to be easier to use the % syntax everywhere for logging instead of reasoning about the performance impact at every logging statement.

self.__class__.__name__,
str(documentId),
str(self.id)))
Expand Down Expand Up @@ -174,8 +174,8 @@ def _index_object(self, documentId, obj, threshold=None, attr=''):
raise
except Exception:
LOG.error('Should not happen: oldDatum was there, now '
'its not, for document with id %s' %
documentId)
'its not, for document with id {0}'.format(
documentId))
Copy link
Member

Choose a reason for hiding this comment

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

Same here.


if datum is not _marker:
self.insertForwardIndexEntry(datum, documentId)
Expand Down Expand Up @@ -203,7 +203,7 @@ def unindex_object(self, documentId):
raise
except Exception:
LOG.debug('Attempt to unindex nonexistent document'
' with id %s' % documentId, exc_info=True)
' with id {0}'.format(documentId), exc_info=True)
Copy link
Member

Choose a reason for hiding this comment

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

Here, too.


def query_index(self, record, resultset=None):
index = self._index
Expand Down
20 changes: 10 additions & 10 deletions src/Products/PluginIndexes/CompositeIndex/CompositeIndex.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ def rawAttributes(self):
return self._attributes

def __repr__(self):
return "<id: %s; metatype: %s; attributes: %s>" % \
(self.id, self.meta_type, self.attributes)
return '<id: {0}; metatype: {1}; attributes: {2}>'.format(
self.id, self.meta_type, self.attributes)
Copy link
Member

Choose a reason for hiding this comment

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

To my mind it would be easier to read if written like this:
'<id: {0.id}; metatype: {0.meta_type}; attributes: {0.attributes}>'.format(self)



@implementer(ITransposeQuery)
Expand All @@ -153,7 +153,7 @@ class CompositeIndex(KeywordIndex):
or sequences of items
"""

meta_type = "CompositeIndex"
meta_type = 'CompositeIndex'

manage_options = (
{'label': 'Settings',
Expand All @@ -162,7 +162,7 @@ class CompositeIndex(KeywordIndex):
'action': 'manage_browse'},
)

query_options = ("query", "operator")
query_options = ('query', 'operator')

def __init__(self, id, ignore_ex=None, call_methods=None,
extra=None, caller=None):
Expand Down Expand Up @@ -304,8 +304,8 @@ def make_query(self, query):
zc = aq_parent(aq_parent(self))
skip = zc.getProperty('skip_compositeindex', False)
if skip:
LOG.debug('%s: skip composite query build %r' %
(self.__class__.__name__, zc))
LOG.debug('{0}: skip composite query build {1}'.format(
self.__class__.__name__, repr(zc)))
Copy link
Member

Choose a reason for hiding this comment

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

Here, too.

return query
except AttributeError:
pass
Expand Down Expand Up @@ -377,7 +377,7 @@ def addComponent(self, c_id, c_meta_type, c_attributes):
# Add a component object by 'c_id'.
if c_id in self._components:
raise KeyError('A component with this '
'name already exists: %s' % c_id)
'name already exists: {0}'.format(c_id))

self._components[c_id] = Component(c_id,
c_meta_type,
Expand All @@ -387,7 +387,7 @@ def addComponent(self, c_id, c_meta_type, c_attributes):
def delComponent(self, c_id):
# Delete the component object specified by 'c_id'.
if c_id not in self._components:
raise KeyError('no such Component: %s' % c_id)
raise KeyError('no such Component: {0}'.format(c_id))

del self._components[c_id]

Expand Down Expand Up @@ -488,8 +488,8 @@ def manage_fastBuild(self, threshold=None, URL1=None,
if RESPONSE:
RESPONSE.redirect(URL1 + '/manage_main?'
'manage_tabs_message=ComponentIndex%%20fast%%20'
'reindexed%%20in%%20%.3f%%20'
'seconds%%20(%.3f%%20cpu)' % (tt, ct))
'reindexed%%20in%%20{0:.3f}%%20'
'seconds%%20({1:.3f}%%20cpu)'.format(tt, ct))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the query string should be constructed using urlencodeinstead of doing it by hand.


manage = manage_main = DTMLFile('dtml/manageCompositeIndex', globals())
manage_main._setName('manage_main')
Expand Down
13 changes: 7 additions & 6 deletions src/Products/PluginIndexes/DateIndex/DateIndex.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,9 @@ def index_object(self, documentId, obj, threshold=None):
except ConflictError:
raise
except Exception:
LOG.error("Should not happen: ConvertedDate was there,"
" now it's not, for document with id %s" %
documentId)
LOG.error('Should not happen: ConvertedDate was there,'
' now it\'s not, for document'
' with id {0}'.format(documentId))
Copy link
Member

Choose a reason for hiding this comment

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

Same here.


if ConvertedDate is not _marker:
self.insertForwardIndexEntry(ConvertedDate, documentId)
Expand Down Expand Up @@ -186,15 +186,16 @@ def _convert(self, value, default=None):

# flatten to precision
if self.precision > 1:
t_val = t_val - (t_val % self.precision)
t_val = t_val - (t_val % self.precision) # noqa: S001
Copy link
Member

Choose a reason for hiding this comment

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

I personally think flake8-pep3101 should not be used. It generates such false positives and there is no way to prevent them.
The % operator for strings is not deprecated and there is no need to switch all string formatting statements to format strings. f-strings are a different league as the additionally provide a performance improvement but we have to wait for them until Python 2 support can be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can hope that this false positive will be solved, s. gforcada/flake8-pep3101#23. Otherwise, wait and see.


t_val = int(t_val)

if t_val > MAX32:
# t_val must be integer fitting in the 32bit range
raise OverflowError(
"%s is not within the range of indexable dates (index: %s)"
% (value, self.id))
('{0} is not within the range of'
' indexable dates (index: {1})'.format(
value, self.id)))
return t_val


Expand Down
34 changes: 17 additions & 17 deletions src/Products/PluginIndexes/DateRangeIndex/DateRangeIndex.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class DateRangeIndex(UnIndex):

security = ClassSecurityInfo()

meta_type = "DateRangeIndex"
meta_type = 'DateRangeIndex'
query_options = ('query', )

manage_options = ({'label': 'Properties',
Expand Down Expand Up @@ -93,46 +93,46 @@ def __init__(self, id, since_field=None, until_field=None,
ceiling_value, precision_value)
self.clear()

security.declareProtected(view, 'getSinceField')
@security.protected(view)
Copy link
Member

Choose a reason for hiding this comment

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

I like this change!

def getSinceField(self):
"""Get the name of the attribute indexed as start date.
"""
return self._since_field

security.declareProtected(view, 'getUntilField')
@security.protected(view)
def getUntilField(self):
"""Get the name of the attribute indexed as end date.
"""
return self._until_field

security.declareProtected(view, 'getFloorValue')
@security.protected(view)
def getFloorValue(self):
""" """
return self.floor_value

security.declareProtected(view, 'getCeilingValue')
@security.protected(view)
def getCeilingValue(self):
""" """
return self.ceiling_value

security.declareProtected(view, 'getPrecisionValue')
@security.protected(view)
def getPrecisionValue(self):
""" """
return self.precision_value

manage_indexProperties = DTMLFile('manageDateRangeIndex', _dtmldir)

security.declareProtected(manage_zcatalog_indexes, 'manage_edit')
@security.protected(manage_zcatalog_indexes)
def manage_edit(self, since_field, until_field, floor_value,
ceiling_value, precision_value, REQUEST):
""" """
self._edit(since_field, until_field, floor_value, ceiling_value,
precision_value)
REQUEST['RESPONSE'].redirect('%s/manage_main'
'?manage_tabs_message=Updated'
% REQUEST.get('URL2'))
REQUEST['RESPONSE'].redirect('{0}/manage_main'
'?manage_tabs_message=Updated'.format(
REQUEST.get('URL2')))

security.declarePrivate('_edit')
@security.private
def _edit(self, since_field, until_field, floor_value=None,
ceiling_value=None, precision_value=None):
"""Update the fields used to compute the range.
Expand All @@ -146,7 +146,7 @@ def _edit(self, since_field, until_field, floor_value=None,
if precision_value not in (None, ''):
self.precision_value = int(precision_value)

security.declareProtected(manage_zcatalog_indexes, 'clear')
@security.protected(manage_zcatalog_indexes)
def clear(self):
"""Start over fresh."""
self._always = IITreeSet()
Expand Down Expand Up @@ -224,7 +224,7 @@ def uniqueValues(self, name=None, withLengths=0):
the form '(value, length)'.
"""
if name not in (self._since_field, self._until_field):
raise StopIteration
return
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I may have ignored backward compatibility, but see https://docs.python.org/3.3/reference/simple_stmts.html#the-return-statement

Copy link
Member

Choose a reason for hiding this comment

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

TIL


if name == self._since_field:
sets = (self._since, self._since_only)
Expand All @@ -248,13 +248,13 @@ def getRequestCacheKey(self, record, resultset=None):
tid = str(term)

# unique index identifier
iid = '_%s_%s_%s' % (self.__class__.__name__,
self.id, self.getCounter())
iid = '_{0}_{1}_{2}'.format(self.__class__.__name__,
self.id, self.getCounter())
# record identifier
if resultset is None:
rid = '_%s' % (tid, )
rid = '_{0}'.format(tid)
else:
rid = '_inverse_%s' % (tid, )
rid = '_inverse_{0}'.format(tid)

return (iid, rid)

Expand Down
21 changes: 18 additions & 3 deletions src/Products/PluginIndexes/DateRangeIndex/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,14 @@ def _checkApply(self, index, req, expectedValues, resultset=None):

def checkApply():
result, used = index._apply_index(req, resultset=resultset)
if hasattr(result, 'keys'):
try:
result = result.keys()
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

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

I like it!

pass

assert used == (index._since_field, index._until_field)
assert len(result) == len(expectedValues), \
'%s: %s | %s' % (req, list(result), expectedValues)
'{0}: {1} | {2}'.format(req, list(result), expectedValues)
for k, v in expectedValues:
assert k in result
return (result, used)
Expand Down Expand Up @@ -148,7 +151,12 @@ def test_retrieval(self):
index = self._makeOne('work', 'start', 'stop')

for i, dummy in dummies:
index.index_object(i, dummy)
result = index.index_object(i, dummy)
self.assertEqual(result, 1)

# don't index datum twice
result = index.index_object(i, dummy)
self.assertEqual(result, 0)

for i, dummy in dummies:
self.assertEqual(index.getEntryForObject(i), dummy.datum())
Expand All @@ -162,6 +170,13 @@ def test_retrieval(self):
self.assertEqual(index.getEntryForObject(result),
match[1].datum())

# check update
i, dummy = dummies[0]
start = dummy._start
dummy._start = start and start + 1 or 1
index.index_object(i, dummy)
self.assertEqual(index.getEntryForObject(0), dummy.datum())

def test_longdates(self):
too_large = 2 ** 31
too_small = -2 ** 31
Expand Down
2 changes: 1 addition & 1 deletion src/Products/PluginIndexes/FieldIndex/FieldIndex.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
class FieldIndex(UnIndex):
"""Index for simple fields.
"""
meta_type = "FieldIndex"
meta_type = 'FieldIndex'
query_options = ('query', 'range', 'not')

manage_options = (
Expand Down
9 changes: 4 additions & 5 deletions src/Products/PluginIndexes/KeywordIndex/KeywordIndex.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#
##############################################################################

import sys
from logging import getLogger

from BTrees.OOBTree import difference
Expand All @@ -37,7 +36,7 @@ class KeywordIndex(UnIndex):

This should have an _apply_index that returns a relevance score
"""
meta_type = "KeywordIndex"
meta_type = 'KeywordIndex'
query_options = ('query', 'range', 'not', 'operator')

manage_options = (
Expand Down Expand Up @@ -135,9 +134,9 @@ def unindex_object(self, documentId):
try:
del self._unindex[documentId]
except KeyError:
LOG.debug('%s: Attempt to unindex nonexistent '
'document with id %s' %
(self.__class__.__name__, documentId),
LOG.debug('{0}: Attempt to unindex nonexistent '
'document with id {1}'.format(
self.__class__.__name__, documentId),
Copy link
Member

Choose a reason for hiding this comment

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

And here.

exc_info=True)

manage = manage_main = DTMLFile('dtml/manageKeywordIndex', globals())
Expand Down
17 changes: 10 additions & 7 deletions src/Products/PluginIndexes/PathIndex/PathIndex.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class PathIndex(Persistent, SimpleItem):
'all docids with this path component on this level'
"""

meta_type = "PathIndex"
meta_type = 'PathIndex'

operators = ('or', 'and')
useOperator = 'or'
Expand Down Expand Up @@ -134,8 +134,10 @@ def unindex_object(self, docid):
""" See IPluggableIndex.
"""
if docid not in self._unindex:
LOG.debug('Attempt to unindex nonexistent document with id %s'
% docid)
LOG.debug('Attempt to unindex nonexistent '
'document with id {0}'.format(
docid))
Copy link
Member

Choose a reason for hiding this comment

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

Here, too.


return

comps = self._unindex[docid].split('/')
Expand All @@ -150,8 +152,9 @@ def unindex_object(self, docid):
if not self._index[comp]:
del self._index[comp]
except KeyError:
LOG.debug('Attempt to unindex document with id %s failed'
% docid)
LOG.debug('Attempt to unindex document '
Copy link
Member

Choose a reason for hiding this comment

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

Here, too.

'with id {0} failed'.format(
docid))

self._length.change(-1)
del self._unindex[docid]
Expand Down Expand Up @@ -277,8 +280,8 @@ def _search(self, path, default_level=0):
if level < 0:
# Search at every level, return the union of all results
return multiunion(
[self._search(path, level)
for level in range(self._depth + 1)])
[self._search(path, lvl)
for lvl in range(self._depth + 1)])

comps = list(filter(None, path.split('/')))

Expand Down
Loading