-
Notifications
You must be signed in to change notification settings - Fork 201
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
Enable extractionFn on Search Filter and Like Filter #209
base: master
Are you sure you want to change the base?
Conversation
@mistercrunch @john-bodley Can you please review this PR? |
requirements-dev.txt
Outdated
@@ -5,3 +5,4 @@ ipdb==0.12 | |||
pip-tools==3.7.0 | |||
pre-commit==1.17.0 | |||
tox==3.11.1 | |||
mock==4.0.2 |
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.
Note this will be resolved via #190.
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'll remove it.
tests/utils/test_filters.py
Outdated
d = filters.Dimension('dim') | ||
actual = filters.Filter.build_filter(d == 'val') | ||
expected = {'type': 'selector', 'dimension': 'dim', 'value': 'val'} | ||
d = filters.Dimension("dim") |
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.
Sorry would you mind disabling black
formatting (or similar) when saving this file as it's hard to easily determine what your change encompasses. I sense black
will probably be enabled for the entire repo in the foreseeable future.
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 understand. I'll revert black in this PR.
And how about we create issue about apply black
?
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.
@SongYunSeop there should definitely be a PR which formats the entire code using black
and adds a CI test to enforce all code is black
formatted going forward.
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.
@john-bodley I created issue(#211) and PR(#212) 😎
@mistercrunch @john-bodley Dose it need any update? |
@SongYunSeop I'm not an admin and thus cannot merge your PR. |
Fixes #208
Addmock
onrequirements-dev.txt
for testing