DOPE-185: extract couchbase module from core #83
Conversation
…ic code into module. - Introduce QueryResolver interface (core): resolves a Resolvable AST with a DopeQueryManager into a backend-specific DopeQuery. - Add new couchbase module with CouchbaseResolver implementing QueryResolver<CouchbaseDopeQuery>: serializes the AST to SQL++, formats clauses, expressions, range operations and aggregates parameters. - Move Couchbase-specific code and tests into the couchbase module (including formatting utilities and FTS search handling); update imports/packages to reference the module. - Wire up Gradle settings to include the couchbase module; keep existing public API unchanged.
…esolver and InfixOperatorResolver so its less clustered
|
@codex review |
| fun build() = toDopeQuery(manager = DopeQueryManager()) | ||
| fun <T : DopeQuery> build(resolver: QueryResolver<T>): T = toDopeQuery(DopeQueryManager(resolver)) | ||
|
|
||
| @Deprecated("Use build(resolver) and pass a backend-specific resolver") |
There was a problem hiding this comment.
That doesnt make sense: Either it is deprecated (= still usable) or it is removed.
Having a method, which just throws an exception doesnt make sense -> support it or remove it
| @@ -0,0 +1,9 @@ | |||
| package ch.ergon.dope.build | |||
There was a problem hiding this comment.
irritating package name, name it ´resolver´
| import ch.ergon.dope.resolvable.Resolvable | ||
|
|
||
| interface QueryResolver<T : DopeQuery> { | ||
| fun resolve(manager: DopeQueryManager<T>, resolvable: Resolvable): T |
There was a problem hiding this comment.
This is not very understandable code: A QueryResolver has a method resolve, which takes a manager with another resolver as a field. So you could have a CouchbaseResolver and with resolve you pass a manager with another CouchbaseResolver? What does this mean? With do we need several?
Either you have a Resolver, which has a manager as a field or a Manager, which has a resolver as a field
There was a problem hiding this comment.
Done, the Resolver has now the manager as a field
| ) | ||
| } | ||
|
|
||
| else -> TODO("Not yet implemented: $clause") |
There was a problem hiding this comment.
Instead of throwing an NotImplementedException use a NotSupportedException
There was a problem hiding this comment.
Please do this for all TODO() in the code
There was a problem hiding this comment.
done, throws UnsupportedOperationException now
| when (resolvable) { | ||
| is Clause -> ClauseResolver.resolve(manager, resolvable) | ||
|
|
||
| is RangeLike<*, *> -> { |
There was a problem hiding this comment.
This File is super long. Can you make more specifi-resolvers (like the ClauseResolver)? E.g. ExpressionResolver, BucketResolver? And then if it still too big, do operatorResolver, typeResolver, ... (Analog to the packages in core) - I move the resolvers in (sub-)packages
We shouldnt have query-strings in this file.
| import ch.ergon.dope.resolvable.expression.type.DopeVariable | ||
| import ch.ergon.dope.validtype.ValidType | ||
|
|
||
| fun <T : ValidType> DopeVariable<T>.toLetDefinitionDopeQuery(manager: DopeQueryManager<CouchbaseDopeQuery>): CouchbaseDopeQuery { |
couchbase/src/main/kotlin/ch/ergon/dope/couchbase/InfixOperatorResolver.kt
Show resolved
Hide resolved
| @@ -0,0 +1,582 @@ | |||
| package ch.ergon.dope.couchbase | |||
There was a problem hiding this comment.
As already commented above, this should be in another sub-package. And the file is too big -> Move it sub-resolvers
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
No description provided.