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.

Sign up for GitHub

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

Jump to bottom

Fix case where multiple indexes with similar name seperated by _ were interpreted as options. #79

Merged
merged 9 commits into from
Jan 27, 2021
Merged

Fix case where multiple indexes with similar name seperated by _ were interpreted as options. #79

merged 9 commits into from
Jan 27, 2021

Conversation

thet
Copy link
Member

@thet thet commented Jun 29, 2019

Fixes #78.

@thet thet requested review from icemac and andbag June 29, 2019 18:41
@thet thet added the bug label Jun 29, 2019
@thet thet mentioned this pull request Jun 29, 2019
Closed
icemac
icemac previously requested changes Jul 1, 2019
View reviewed changes
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

Thank you for tackling the issue.
This fix breaks some tests.

@icemac
Copy link
Member

icemac commented Oct 9, 2020

Are there any news on this PR?

andbag
andbag reviewed Oct 9, 2020
View reviewed changes
Copy link
Member

@andbag andbag left a comment

Choose a reason for hiding this comment

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

If the ambiguity is avoided, the PR is fine.

@petschki petschki mentioned this pull request Jan 22, 2021
Closed
@petschki
Copy link
Contributor

Just pushed the ambiguity suggestion. This fixes #114 too

@icemac
Copy link
Member

icemac commented Jan 22, 2021

After #115 is merged we can run the tests again in CI.

agitator added a commit that referenced this pull request Jan 22, 2021
@agitator
pick fix from #79 to fix #114
3ce8da5
@petschki petschki requested a review from icemac January 27, 2021 10:41
ale-rt
ale-rt requested changes Jan 27, 2021
View reviewed changes
@petschki petschki requested a review from ale-rt January 27, 2021 10:57
ale-rt
ale-rt approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

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

LGTM

@icemac icemac dismissed their stale review January 27, 2021 13:19

Tests are green now.

icemac
icemac approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

LGTM beside a singe improvement suggestion.

@icemac icemac mentioned this pull request Jan 27, 2021
Merged
@petschki petschki merged commit 2cefd07 into master Jan 27, 2021
@petschki petschki deleted the thet-fix-78 branch January 27, 2021 14:14
agitator added a commit that referenced this pull request Jan 28, 2021
@agitator
udpate with changes from #79
cefaef6
icemac pushed a commit that referenced this pull request Jan 29, 2021
@agitator
pick fix from https://github.com/zopefoundation/Products.ZCatalog/pul… (
91e7826 
#116)

* pick fix from #79 to fix #114
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petschki petschki petschki left review comments

@ale-rt ale-rt ale-rt approved these changes

@andbag andbag andbag left review comments

@icemac icemac icemac approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Query for multiple indexes with similar names broken
6 participants
@thet @icemac @petschki @ale-rt @andbag @agitator