Fix deprecation warnings #38
Conversation
There was a problem hiding this comment.
@andbag Thank you for your work, even on the small details. But I have to mention, that reading the big PR was quite hard. It would be really helpful for me, if you could separate the changes semantically in different PRs (like the single/double quote change), which would it make easier to approve and merge.
The only thing really to change is the superfluous % at one part.
@icemac What is you opinion on the asserts? Should we have here the specific versions or is it okay for now to stay with self.assertTrue? I have just marked all places I found.
| @@ -50,7 +50,7 @@ def test_index_document(self, docid=1): | |||
| self.index.length()) | |||
| for map in self.index._wordinfo.values(): | |||
| self.assertEqual(len(map), 1) | |||
| self.assert_(docid in map) | |||
| self.assertTrue(docid in map) | |||
There was a problem hiding this comment.
This could have been rewritten as `self.assertIn(docid, map).
| self.assert_(1 in map) | ||
| self.assert_(docid in map) | ||
| self.assertTrue(1 in map) | ||
| self.assertTrue(docid in map) |
There was a problem hiding this comment.
These two lines could also use assertIn.
| self.assertEqual(len(self.index._wordinfo), 4) | ||
| self.assertEqual(len(self.index._docwords), 1) | ||
| self.assertEqual(len(self.index.get_words(docid)), 4) | ||
| self.assertEqual(len(self.index._wordinfo), | ||
| self.index.length()) | ||
| for map in self.index._wordinfo.values(): | ||
| self.assertEqual(len(map), 1) | ||
| self.assert_(docid in map) | ||
| self.assertTrue(docid in map) |
There was a problem hiding this comment.
This line could also use assertIn.
| self.assertEqual(len(wids), 1) | ||
| for wid, map in self.index._wordinfo.items(): | ||
| self.assertEqual(len(map), 1) | ||
| self.assert_(docid in map) | ||
| self.assertTrue(docid in map) |
There was a problem hiding this comment.
This line could also use assertIn.
| @@ -271,12 +271,12 @@ def test_upgrade_document_count(self): | |||
| del self.index1.document_count | |||
| self.index1.index_doc(1, 'gazes upon my shadow') | |||
| self.index2.index_doc(1, 'gazes upon my shadow') | |||
| self.assert_(self.index1.document_count.__class__ is Length) | |||
| self.assertTrue(self.index1.document_count.__class__ is Length) | |||
There was a problem hiding this comment.
This line could also use assertIs.
|
|
||
| def testDelColumn(self): | ||
| self._catalog.addColumn('title') | ||
| self._catalog.delColumn('title') | ||
| self.assert_('title' not in self._catalog.schema()) | ||
| self.assertTrue('title' not in self._catalog.schema()) |
There was a problem hiding this comment.
This line could also use assertNotIn.
|
|
||
| def testAddColumn(self): | ||
| self._catalog.addColumn('num', default_value=0) | ||
| self.assert_('num' in self._catalog.schema()) | ||
| self.assertTrue('num' in self._catalog.schema()) |
There was a problem hiding this comment.
This line could also use assertIn.
| '%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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)), | ||
| LOG.error('{0}: unindex_object could not remove ' | ||
| 'documentId %{1} from index {2}. This ' |
| return default | ||
|
|
||
| self._hits += 1 |
There was a problem hiding this comment.
I do not understand, why this method used to set _misses and _hits, but does not need it anymore. As it seems to be actually a tool for testing, it should be fine, if nothing breaks.
There was a problem hiding this comment.
The "get" method now calls the"getitem" method (see self[key]), which still sets the variables"_hits" and"_misses". Other tests using the method will therefore continue to work.
|
@sallner Sorry and thank you for your tips and corrections. Next time I'll structure the PR better. |
There was a problem hiding this comment.
In general I like the changes you did and the effort you put into it.
As you touched the logging statements, I really like to see that they get changed to the way the logging module was intended. (I marked them all to be able to find them in the PR comments as outdated comments after changes.)
I prefer to use more specific assert methods. (They were mostly not yet existing when the code was written.)
I am not sure if changing all double quotes to single quotes is worth the effort. The Python documentation does not prefer one over the other (https://docs.python.org/3/library/stdtypes.html#textseq) and the Python style guide for ZTK packages does not mention quotes at all: http://zopetoolkit.readthedocs.io/en/latest/codingstyle/python-style.html. (There does not seem to be a general style guide for Zope Foundation packages.) But I am okay with keeping these quoting changes as they do not hurt.
| '%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( |
There was a problem hiding this comment.
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.
| 'its not, for document with id %s' % | ||
| documentId) | ||
| 'its not, for document with id {0}'.format( | ||
| documentId)) |
| @@ -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) | |||
| 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) |
There was a problem hiding this comment.
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)
| 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))) |
| @@ -145,7 +145,7 @@ def test_add_field_index(self): | |||
| catalog = self._make_one() | |||
| idx = FieldIndex('id') | |||
| catalog.addIndex('id', idx) | |||
| self.assert_(isinstance(catalog.indexes['id'], FieldIndex)) | |||
| self.assertTrue(isinstance(catalog.indexes['id'], FieldIndex)) | |||
| @@ -325,7 +328,7 @@ def testKeywordIndexWithMinMaxRangeWrongSyntax(self): | |||
| # checkKeywordIndex with min/max range wrong syntax. | |||
| catalog = self._make_one() | |||
| a = catalog(att3={'query': ['att'], 'range': 'min:max'}) | |||
| self.assert_(len(a) != self.upper) | |||
| self.assertTrue(len(a) != self.upper) | |||
|
|
||
| def test_start(self): | ||
| plan = self._makeOne() | ||
| plan.start() | ||
| self.assert_(plan.start_time <= time.time()) | ||
| self.assertTrue(plan.start_time <= time.time()) |
| so = plan.interim['sort_on'] | ||
| self.assert_(so.start <= so.end) | ||
| self.assert_('sort_on' not in plan.benchmark) | ||
| self.assertTrue(so.start <= so.end) |
| self.assert_('index2' in plan.benchmark) | ||
| self.assertEquals(plan.benchmark['index2'].hits, 0) | ||
| self.assertEquals(set(plan.plan()), set(('index1', 'index2'))) | ||
| self.assertTrue(plan.duration > 0) |
There was a problem hiding this comment.
This could also be called with assertGreater.
setup.cfg
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
flake8-plone-api should not be used on this repos, so ignoring P001 should not be necessary.
[skip ci]
Unfortunately I have currently no time to increase the code coverage of the tests.