Skip to content
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

Refactoring QueryStringSearchOperatorDsl #58

Closed
wants to merge 25 commits into from
Closed

Conversation

masonJS
Copy link
Contributor

@masonJS masonJS commented Sep 30, 2023

Hi, Jake Son.
I've made a few edits to the work I've done.
Please check and approve.

Comment on lines 46 to 48
query = Query("\"$escaped\"", field)

return query
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this idea is great.
but there is another solution.

we can change QueryStringSearchOperatorDsl.kt like this.

    fun query(configuration: QueryStringQueryOptionDsl.() -> QueryStringQueryOptionDsl.Query) {
        val query = QueryStringQueryOptionDsl().configuration()
        document["query"] = query.toString()
    }

We no longer need to reassign the query in every function by using this method.
what do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion.
I'm going to ask you again just to make sure I understand what you mean.

If I write it as you say,
the queryproperty that existed in the QueryStringQueryOptionDslclass will disappear?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and build method also removed.

* @param rightInclusion The right value is included in the range
* @param field Indexed field to search
*/
fun range(left: String, right: String, leftInclusion: Boolean = true, rightInclusion: Boolean = true, field: KProperty<String?>): Query {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍 👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants