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

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

Closed
wants to merge 3 commits into from

Conversation

razvanMiu
Copy link

No description provided.

@icemac
Copy link
Member

icemac commented Mar 1, 2024

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
Copy link
Author

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

@razvanMiu razvanMiu requested a review from icemac March 26, 2024 14:41
@icemac icemac requested review from dataflake and d-maurer and removed request for icemac March 28, 2024 06:33
@icemac
Copy link
Member

icemac commented Mar 28, 2024

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

Copy link
Member

@dataflake dataflake left a comment

Choose a reason for hiding this comment

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

I'm afraid I cannot help reviewing this. I don't have much knowledge about the inner workings of the catalog.

@ale-rt
Copy link
Member

ale-rt commented Mar 28, 2024

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?

Copy link
Contributor

@d-maurer d-maurer left a comment

Choose a reason for hiding this comment

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

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?

@razvanMiu
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants