-
Notifications
You must be signed in to change notification settings - Fork 12
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: handle datetime in nosql_apply_parameters_to_query #299
feat: handle datetime in nosql_apply_parameters_to_query #299
Conversation
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.
Great! Here is one small question for something I've trouble understanding :
{'column': 'date', 'operator': 'in', 'value': '{{ allowed_dates }}'}, | ||
{'allowed_dates': [datetime(2020, 12, 31), datetime(2021, 1, 1)]}, | ||
{ | ||
'column': 'date', | ||
'operator': 'in', | ||
'value': [datetime(2020, 12, 31), datetime(2021, 1, 1)], |
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.
Great for checkboxes variables 👍
clean_p = deepcopy(parameters) | ||
|
||
# Replace param templating with jinja templating: | ||
query = re.sub(RE_PARAM, r'{{ \g<1> }}', query) |
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.
So easier ^^
By the way, I think we should progressively retire this parameter syntax in favor of Jinja one, which is simpler to read and write. Also, it's complex for users to have two possibilities that does the same thing.
Creating an issue to suggest that: #303
return res | ||
env = NativeEnvironment() | ||
res = env.from_string(query).render(clean_p) | ||
# NativeEnvironment's render() isn't recursive, so we need to |
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.
It's not instantly clear to me what do you mean by "render is not recursive".
May you explain that with an example? Thx
(I tried this to test nested arrays, but obtained as expected:
>>> env.from_string('{{ [punk, [ipa]] }}').render({'punk': ['pale', 'ale'], 'ipa': 10})
[['pale', 'ale'], [10]]
)
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.
Sure !
There are actually 2 problems :
The 1st one is that render
will apply a ast.literal_eval
on the result (if the result is a str
), and by doing so we lose the original type :
>>> env.from_string('{{ x }}').render({'x': 1}) # we expect 1
1
>>> env.from_string('{{ x }}').render({'x': '1'}) # we expect '1'
1 # <- meh
to handle this problem, a solution is to put strings inside quotes, this is what _prepare_parameters
does.
>>> env.from_string('{{ x }}').render({'x': '"1"'}) # we expect '1'
'1' # <- it works !
it also works if x is a list and we get one element from this list :
>>> env.from_string('{{ x[0] }}').render({'x': ['"1"', '"2"']}) # we expect '1'
'1'
but here's the 2nd problem :
>>> env.from_string('{{ x }}').render({'x': ['"1"', '"2"']}) # we expect ['1', '2']
['"1"', '"2"'] # <- meh
in this case, since the result is a list
and not a str
, jinja did not apply the literal_eval
recursively. That's why we have to do it by hand, by calling _prepare_result
.
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.
LGTM
Before
would return
(there was a forced cast to string, making it impossible to keep the date type)
After
it returns
(you can use
bar | string
orbar.isoformat()
orbar.strftime(args)
to get a string output)