Ts vector and ts query accepts language #15
Conversation
|
hey @kaj, sorry for the late response. The LICENSE file was required by my company in order to use the crate. I've excluded from this current PR and I'll create a proper issue/PR with the license. Let me know if you need anything else. |
There was a problem hiding this comment.
Looks fine module that semi related requested change there. It would be quite great to fix this minor issue along the way 😉
As another node: This needs to change the minimal supported diesel version, as the new sql_function! syntax is only available since 1.3.0. (Also I'm unsure if such a minimal version bump would require a major version bump for this crate. In this case we need to wait at least till the release of diesel 2.0 to issue a new release here)
src/lib.rs
Outdated
| #[derive(Clone, Copy)] | ||
| pub struct TsQuery; | ||
|
|
||
| #[derive(Clone, Copy)] | ||
| pub struct TsVector; |
There was a problem hiding this comment.
Can you change that to use #[derive(SqlType)] as well? This would fix #19
(Requires removing the explicit HasSqlType and NotNull impls below.
There was a problem hiding this comment.
I've updated, but I confess that I don't know the meaning of exactly what I'm doing =/
There was a problem hiding this comment.
And @weiznich sorry but I didn't get what you meant in Looks fine module that semi related requested change there. It would be quite great to fix this minor issue along the way
Which module/which change/which fix?
There was a problem hiding this comment.
It's a type, it's meant to be "modulo" instead of "module". So everything beside those small things is fine (+ the version bump for diesel + discussing how we should handle that)
There was a problem hiding this comment.
This change itself looks fine, but we need to bump the minimal supported diesel version to 1.3.0 to make this work as this is the first version that supports the new sql_function! syntax. I'm not sure how those bump would interact with semver constrains. Would this require a major version bump of diesel_full_text_search (in this case we should wait with releasing this change till the diesel 2.0 release) or would be a minor version bump be acceptable?
|
I think that a since it is a minor version change the diesel requirement, it should be ok to do it on a minor (but not patch) version change in this crate. But the change in diesel requirement (in |
|
Is just this that needs to be done? (and then someone that has authorisation does a |
|
This needs to fix the minimal supported diesel version here and I want to hear the opinion of @diesel-rs/contributors first on the sem version concerns. |
|
Ok. I've pushed as if we would bump the version and minimum diesel version. Let me know if I should revert after you hear from them. |
|
Strange; github says this branch has no conflicts with the base branch, but the "old" code in the diff looks a lot different from what I see when looking at the master branch directly. Also the functions |
|
Also, the update of the diesel dependency that is part of this PR should probably have been a part of #13 ? |
|
@kaj thanks a lot. I don't know exactly where you're seeing this but think initially I applied I agree about the bump version in the other PR, but I didn't have any idea about that at the time. feedback about the comments (don't get me wrong, but I don't know exactly what to do here): is there an action item to be done or not? |
I don't really know what would happen if this is merged as is, maybe there would be need for a manual merge (even if github claims not) and maybe it would remove the recently added support for I think it would be good to rebase this PR on the current master before merging, just to be safe. |
mrcosta
force-pushed
the
ts_vector_and_ts_query_accepts_language
branch
from
39bca60
to
f84220e
Compare
|
You were right @kaj. I think now is ok regarding |
closes #14