-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
Support Multiple Catalogs for Dexterity Contents #1489
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small performance bit
for rc in REQUIRED_CATALOGS: | ||
if rc in catalogs: | ||
continue | ||
catalogs.append(rc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the following be easier?:
catalogs = getattr(obj, "_catalogs", []) + REQUIRED_CATALOGS
catalogs = list(set(catalogs))
return map(api.get_tool, catalogs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current approach is more readable and performant, because in your approach the list hast to be iterated multiple times to generate first a set
and convert it then to a list
.
But indeed, it would be interesting what approach is faster...
... 5 minutes later ...
Here comes the proof:
REQUIRED_CATALOGS = [
"auditlog_catalog",
]
def v1(obj):
catalogs = getattr(obj, "_catalogs", [])
for rc in REQUIRED_CATALOGS:
if rc in catalogs:
continue
catalogs.append(rc)
return catalogs
def v2(obj):
catalogs = getattr(obj, "_catalogs", []) + REQUIRED_CATALOGS
catalogs = list(set(catalogs))
return catalogs
class Content(object):
_catalogs = ["setup_catalog"]
obj = Content()
if __name__ == '__main__':
from timeit import timeit
print timeit("v1(obj)", setup="from __main__ import obj, v1")
print timeit("v2(obj)", setup="from __main__ import obj, v2")
Output:
0.572149038315
0.9786028862
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pheew!
.format(catalog.id, url)) | ||
# We want the intersection of the catalogs idxs | ||
# and the incoming list. | ||
indexes = set(catalog.indexes()).intersection(attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If attributes have been manually specified, but no matches with current catalog are found, the object will be reindexed for all indexes. I would rather skip this case for better performance:
indexes = set(catalog.indexes()).intersection(attributes)
if attributes and not indexes:
continue
logger.info(
"CatalogMultiplexProcessor::indexObject:catalog={} url={}"
.format(catalog.id, url))
catalog.catalog_object(obj, url, idxs=list(indexes))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, will change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Description of the issue/feature this PR addresses
This PR implements an
IIndexQueueProcessor
to multiplex Dexterity indexing to multiple catalogsUsage
Add the
bika.lims.interfaces.IMultiCatalogBehavior
to the Dexterity type config:Add the supported catalogs into an
_catalogs
attribute of your Dexterity content:The catalog multiplexer will automatically index the content in the requested catalogs.
Current behavior before PR
Dexterity contents are only indexed in
portal_catalog
Desired behavior after PR is merged
Dexterity contents are indexed in
portal_catalog
and additional catalogs--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.