-
Notifications
You must be signed in to change notification settings - Fork 20
(dsl): Support intervalQuery #647
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
base: main
Are you sure you want to change the base?
Conversation
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/test/scala/zio/elasticsearch/ElasticQuerySpec.scala
Outdated
Show resolved
Hide resolved
|
Another thing - change PR title as well as reference to the issue you're resolving. |
…twith, endwith), Regex instead of String in intervalRegexp, remove useless imports
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
| * @param query | ||
| * the text to match in the intervals. |
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.
| * @param query | |
| * the text to match in the intervals. | |
| * @param query | |
| * the text to match in the intervals |
| * @param prefix | ||
| * the prefix string to match. |
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.
| * @param prefix | |
| * the prefix string to match. | |
| * @param prefix | |
| * the prefix string to match |
| /** | ||
| * Represents a `match` interval query. Matches analyzed text within intervals. | ||
| * | ||
| * @param query | ||
| * The text to match. | ||
| * @param analyzer | ||
| * Optional analyzer to use for this query. | ||
| * @param useField | ||
| * Optional alternative field used for term extraction. | ||
| */ | ||
|
|
||
| /** `match` interval-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.
This shouldn't be here. We are using scaladoc only for public api methods.
| * an instance of [[zio.elasticsearch.query.IntervalMatch]] that represents the `match` interval query. | ||
| */ | ||
|
|
||
| final def intervalMatch(query: String): IntervalMatch[String] = |
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.
We should have specific trait here as return type for methods here. (you can see other methods)
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
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.
Good first PR.
Few more things to do besides other comments:
- Rename PR
- Add IT tests
- Add documentation
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
modules/library/src/test/scala/zio/elasticsearch/ElasticQuerySpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
modules/library/src/test/scala/zio/elasticsearch/ElasticIntervalQuerySpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/test/scala/zio/elasticsearch/ElasticIntervalQuerySpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/test/scala/zio/elasticsearch/ElasticIntervalQuerySpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/test/scala/zio/elasticsearch/ElasticQuerySpec.scala
Outdated
Show resolved
Hide resolved
|
I have a problem with scalafixAll and scalafmtAll, after formatting it tells me that everything is not formatted. |
modules/library/src/test/scala/zio/elasticsearch/ElasticQuerySpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/test/scala/zio/elasticsearch/ElasticQuerySpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/test/scala/zio/elasticsearch/ElasticQuerySpec.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/Queries.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/options/HasUseField.scala
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/ElasticQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalQuery.scala
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,30 @@ | |||
| /* | |||
| * Copyright 2022 LambdaWorks | |||
| * | |||
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.
I see that you resolved this comment, but I still see some strange chars here.
modules/library/src/main/scala/zio/elasticsearch/query/options/HasAnalyzer.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/options/HasUseField.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/options/HasUseField.scala
Outdated
Show resolved
Hide resolved
| Alternatively, you can pass a `Field` object directly: | ||
| ```scala | ||
| val queryWithFieldObject: IntervalQuery[Document] = | ||
| intervals(field = "content", rule = intervalMatch("targetWord").useField(Document.stringField)) | ||
| ``` |
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.
This seems like duplicate example of queryWithField.
| /* | ||
| * Copyright 2022 LambdaWorks | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
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.
You have resolved this comment, but you are still missing copyright here.
| query = | ||
| term(field = TestDocument.stringField, value = secondDocumentUpdated.stringField.toLowerCase) |
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.
This is also resolved, but there is still unnecessary change here.
modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala
Outdated
Show resolved
Hide resolved
| } | ||
| /* | ||
| * Copyright 2022 LambdaWorks | ||
| * |
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.
There are still weird chars here.
modules/library/src/test/scala/zio/elasticsearch/ElasticIntervalQuerySpec.scala
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
| /* | ||
| * Copyright 2022 LambdaWorks | ||
| * | ||
| * |
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.
this is unnecessary
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
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.
please remove this
| /* | ||
| * Copyright 2022 LambdaWorks | ||
| * | ||
| * |
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.
please remove this
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.
Please fix this file
drmarjanovic
left a comment
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.
Two additional notes:
- website generating fails
- tests fail for Scala 2.12 and Java 17
modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala
Outdated
Show resolved
Hide resolved
modules/integration/src/test/scala/zio/elasticsearch/HttpExecutorSpec.scala
Outdated
Show resolved
Hide resolved
| val docWithTerm = doc.copy(stringField = s"$term banana orange") | ||
|
|
||
| val query = intervals( | ||
| "stringField", |
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.
Please use TestDocument.stringField.
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.
Is there any issue with this?
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
| val docWithTerm = doc.copy(stringField = s"$term banana orange") | ||
|
|
||
| val query = intervals( | ||
| "stringField", |
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.
Is there any issue with this?
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
modules/library/src/main/scala/zio/elasticsearch/query/ElasticIntervalRule.scala
Outdated
Show resolved
Hide resolved
dbulaja98
left a comment
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.
Please rename ElasticIntervalQuery.scala and ElasticIntervalQuerySpec because it's actually rule, not query.
And, please add unit tests for the query itself
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.
Should this be also renamed to Rule?
Part of #91