⭐ Improve parameterized query support - fixes #793 #794
Conversation
pypika/terms.py
Outdated
| @property | ||
| def placeholder(self): | ||
| if callable(self._placeholder): | ||
| return self._placeholder(None) |
There was a problem hiding this comment.
how does this work on None?
There was a problem hiding this comment.
callable Returns False
But I presume you mean how will the self_placeholder(None) work with None.
Good point, it wouldn't atm with the default generators.
I will fix it now.
|
Thanks for the PR! Will take a deeper look later |
@wd60622 Do you think you'll have time this week? |
| def named_placeholder_gen(idx: int) -> str: | ||
| return f'param{idx + 1}' | ||
|
|
||
|
|
||
| class Parameter(Term): | ||
| is_aggregate = None | ||
|
|
||
| def __init__(self, placeholder: Union[str, int]) -> None: |
There was a problem hiding this comment.
Type hint here doesn't include callable? Is that correct still?
There was a problem hiding this comment.
Correct, only ListParameter and DictParameter (and classes that inherit from them) support callable.
The base parameter does not.
| class Parameter(Term): | ||
| is_aggregate = None | ||
|
|
||
| def __init__(self, placeholder: Union[str, int]) -> None: | ||
| super().__init__() | ||
| self.placeholder = placeholder | ||
| self._placeholder = placeholder |
There was a problem hiding this comment.
Might be good to use a setter here. With that, implementing a check for the correct type can happen on initialization if put in the setter
There was a problem hiding this comment.
I'm not sure I follow how to do this pythonically? Could you share some pseudo code to show what you mean?
There was a problem hiding this comment.
@mvanderlee I believe @wd60622 was referring to the following:
@property
def placeholder(self):
return self._placeholder
@placeholder.setter
def placeholder(self, placeholder):
# can add checks here if needed
self._placeholder = placeholderThere was a problem hiding this comment.
Right, which adds no value and isn't pythonic. This isn't Java/C#.
It would also break parameters. After a parameter has been created and used, any changes to the placeholder will break queries. Which is why I've made it private with a getter only.
I haven't changed the type, so any type checks would have to be part of a separate PR.
There was a problem hiding this comment.
I would disagree with you on it not being Pythonic, as it’s a native language feature.
Also, I was trying to be helpful, but given your receptiveness to discussion and tone, I won’t proceed.
Good luck.
There was a problem hiding this comment.
Sorry for my tone @danielenricocahall it'd been a rough week and I lashed out.
As for pythonic. This video does a good job at explaining language feature vs pythonic.
Raymond Hettinger - Beyond PEP 8 -- Best practices for beautiful intelligible code - https://www.youtube.com/watch?v=wf-BqAjZb8M
|
Do you have any thoughts as well? @AzisK |
|
Could we also have this functionality described in pypika docs? |
|
The tests look really nice. What about adding |
Fixes #793
The implemented tests speak for themselves, but here is an extract:
This would allow the user to easily plug into SQLAlchemy for example: