Add String functions(substring) #538
Conversation
waahhh
requested review from
pickmoment,
huisam,
cj848 and
shouwn
as code owners
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #538 +/- ##
===========================================
+ Coverage 91.88% 91.95% +0.06%
===========================================
Files 285 287 +2
Lines 3106 3145 +39
Branches 189 193 +4
===========================================
+ Hits 2854 2892 +38
- Misses 197 198 +1
Partials 55 55 ☔ View full report in Codecov by Sentry. |
| fun substring( | ||
| value: Expression<String>, | ||
| start: Int, | ||
| length: Int? = null, | ||
| ): Expression<String> { | ||
| return Expressions.substring( | ||
| value, | ||
| Expressions.value(start), | ||
| length?.let { Expressions.value(length) }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
For user convenience, I see that you've created a two-parameter combination of an Expressionable and a primitive type. Obviously, I see this as a great convenience over maintenance cost.
Previously, when implementing the like side, I requested that functions that were a combination of an Expressionable and a primitive type be removed for maintenance, but this expression made me think differently.
It would be much better for users to have escape as the default type for like as well.
Thanks again for your help and this new insight.
사용자 편의성을 위해 Expressionable과 기본 자료형의 조합 표현식을 만들어주신 것이군요. 확실히 이 조합은 유지보수 비용 대비해서 사용자에게 많은 편의성을 제공할 것 같습니다.
이전에 like 쪽을 구현할 때 제가 Expressionable과 기본 자료형의 조합을 유지보수 비용의 증가 때문에 제거를 요청드렸는데요. 이 케이스를 보니 생각이 달라지게 되었습니다.
그것도 사용자 편의성을 위해 escape가 기본 자료형인 것이 훨씬 이점이 크겠네요.
도와주셔서 감사하고 새로운 인사이트를 주셔서 감사합니다!
There was a problem hiding this comment.
근데 여기도 Expressionable 를 사용하는게 맞지 않나요?
There was a problem hiding this comment.
사용자 편의성을 위해 Expressionable과 기본 자료형의 조합 표현식을 만들어주신 것이군요. 확실히 이 조합은 유지보수 비용 대비해서 사용자에게 많은 편의성을 제공할 것 같습니다.
fun substring(
value: Expressionable<String>,
start: Int,
length: Int? = null,
): Expression<String> {...}이전 리뷰해 주신 내용들을 생각해봤을때, 위 형태의 메서드를 제공하는게 맞을지 좀 고민됐는데
공감해주셔서 감사합니다. 😀
추가로 concat function 지원을 위한 구현 중에
fun concat(
value1: String,
value2: String,
vararg others: String
): Expression<String> {...}
fun concat(
value1: Expressionable<String>,
value2: String,
vararg others: String
): Expression<String> {...}
fun concat(
value1: String,
value2: Expressionable<String>,
vararg others: String
): Expression<String> {...}
fun concat(
value1: Expressionable<String>,
value2: Expressionable<String>,
vararg others: Expressionable<String>
): Expression<String> {...}
위와 같은 형태의 메서드들을 제공하려고했었는데, 괜찮을까요?
사용자마다 사용 패턴은 다양할테고, 많이 사용하는 패턴에 대한 객관적인 자료가없다보니
편의성의 기준도 모호한 부분이 있고, 모든 케이스를 대응하는건 유지보수 차원에서 부담되지만
이정도 편의성은 제공해야하지 않을까 싶긴한데 의견 주시면 반영하겠습니다.
Expressionable 관련 리뷰는 반영했습니다!
There was a problem hiding this comment.
결정하기 어렵네요... concat의 경우에는 모두 Expressionable인 것과 모두 기본 자료형인 것 두 개로 가는 게 좋을 것 같아요.
이번 PR을 통해서 사용자 편의성에 대해서 다시 생각을 해보게 되었는데요. substring의 케이스는 왜 괜찮고 concat의 경우는 왜 안 괜찮다고 생각했는지 고민해보았습니다.
제 기준은 아마도 파라미터가 가지는 의미성에 있는 것 같아요.
concat이 가지는 파라미터는 모두 동일한 의미성을 가진 파라미터이기 때문에 사용자가 바라보는 시선이 동일할 것이고, 하지만 substring의 경우는 start와 length는 value와는 아예 다른 성격의 파라미터이기 때문에 타입이 다른 것이 섞여 있어도 괜찮다고 생각한 것 같습니다.
작성해주신 concat의 스펙을 용인하면 어떤 사용자가 value1: Expressionable<String>, value2: String, value3: Expressionable<String>, value4: String를 요청했을 때에도 그 요청을 안 들어줄 이유가 없어지기 때문에 그런 것 같네요.
There was a problem hiding this comment.
작성해주신 concat의 스펙을 용인하면 어떤 사용자가
value1: Expressionable<String>, value2: String, value3: Expressionable<String>, value4: String를 요청했을 때에도 그 요청을 안 들어줄 이유가 없어지기 때문에 그런 것 같네요.
저도 남겨주신 의견을 보고 다시 생각해보니 kotlin-jdsl에서는 저런 다양한 스펙까지 제공 안 해줘도 �다른 jpql빌더들에 비해 간단히 여러 케이스에 대응이 가능한데도, 제가 너무 사용자 편의성 관점에 치중해버린 것 같네요. 저도 어쩔수없는 편의에 굶주린 사용자인가 봅니다..😓 구현을 하다 보니 스쳐 지나간 concat의 사용 사례들이 떠올라 최소한 이 정도 스펙만 추가해줘도 대부분?의 사례에서 좀 더 깔끔하고 편히 쓸 수 있을 것 같다는 생각이 들어버렸네요.. 😰 이전부터 말씀해주신 기본정책(모두 Expressionable인 것과 모두 기본형�)을 가져가면서, 옵션?류의 파라미터에 한해 상황에 따라 일부 편의성을 제공하는 게 앞으로의 유지보수 측면이나 사용하는 입장에서도 일관성있고 좋을 것 같네요! 추후에 기존 기능을 deprecated 시키는 것보단, 정말 필요하다면 새롭게 추가하는 게 더 수월할 테고요! 👍
| value: Expression<String>, | ||
| start: Expression<Int>, | ||
| length: Expression<Int>? = null, |
There was a problem hiding this comment.
Please use a Expressionable.
Expressionable로 작성을 부탁드려요!
waahhh
force-pushed
the
feature/add-string-functions-substring
branch
from
db983e6 to
be8a15c
Compare
Motivation
Modifications
Commit Convention Rule
commit typeplease describe it on the Pull RequestResult
Closes