search

zopefoundation / Products.ZCatalog

Zope's indexing and search solution.
Other
5 stars 22 forks source link

feat(catalog): add rids parameter to return only the rids of documents #149

Closed razvanMiu closed 5 months ago

icemac commented 6 months ago

Thank you for your contribution.

According to the contributing policies of the zopefoundation organization you need to sign a contributor agreement before any non-trivial change can be merged. For details please consult the Contributing guidelines for zopefoundation projects.

razvanMiu commented 5 months ago

@icemac Can you take a look now at this pr?

icemac commented 5 months ago

@razvanMiu I assigned two developers for review which know more about this package.

ale-rt commented 5 months ago

I am always shy changing the signature of methods that accept ** keyword arguments. This would be a breaking change for whoever (if they exist) have a rids index. Instead of changing the signature and the return type of searchResults, did you consider adding another method, e.g. searchRids?

razvanMiu commented 5 months ago

I do not like the PR:

  • it comes from a foreign fork rather than a branch of the main repository -- this makes a detailed analysis more difficult (and sometimes provokes test suite problems)
  • it lacks documentation: no CHANGES.rst entry; no reference to the new feature in the source code docstrings; no indication how the new parameter interacts with sorting
  • no tests associated with the new feature

The new parameter is similar to merge=False; would you explain why you need the new parameter?

I'll close this pr, make a new branch with a different approach and explain in details why this is needed.