-
Notifications
You must be signed in to change notification settings - Fork 14
Update Facets Subset Logic #50
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
Conversation
Pull Request Test Coverage Report for Build 20437227736Details
💛 - Coveralls |
|
|
anthonyp97
left a comment
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.
Looks great! Do you know the procedure for creating a new version of this in pypi: https://pypi.org/project/django-redis-autocompleter/
|
|
||
| facet_list = facet_group["facets"] | ||
| facet_group_keys_set = set([sub_facet["key"] for sub_facet in facet_list]) | ||
| if not facet_group_keys_set.issubset(provider_keys_set): |
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.
Confirming my understanding: so we are keeping the subset logic check but instead of checking all facets passed in is a subset of the provider's facets, we now check each facet group passed in is a subset of the provider facets. So we need to make sure we pass in new facet groups if we want them to work with providers that have unique facets
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.
Yeah exactly, each 'and' or 'or' facet group will be treated independently of each other; for each facet group we'll check to see if it's a subset of provider facets to see whether we can evaluate and use that particular facet group.
|
@anthonyp97 bumped up the version in |
|
@herrmann-e so you first merge this to master and then we follow these steps: https://github.com/ycharts/django-autocompleter/blob/master/DEVELOPMENT.md. Feel free to merge and I can help with pushing to pypi |
|
@anthonyp97 merged but may need help with the pypi stuff yeah 🙏 |
|
@herrmann-e cool I just uploaded the new version to pypi using our token: Check it out here: https://pypi.org/project/django-redis-autocompleter/ |
Overview
For each facet group (each set of keys joined by an "or" or "and"), we should decide whether to use it based on whether the keys are a subset of the facets that the provider defines. This is now on a facet group level, not on the level of the entire list of facet groups.
If we have the following:
then:
How to test
yc_ssh&python manage.py autocompleter --store --remove --clear_cache --name='separate_account'.docker/docker-compose.ymlto update the web step (can copy/paste)requirements.txtto remove the lines:yc_django