Skip to content

Simplify the code in the QueryClauses class - #86

Merged
sergio-bobillier merged 1 commit into
masterfrom
sb-simplify-QueryClauses
Oct 27, 2026
Merged

sergio-bobillier merged 1 commit into
masterfrom
sb-simplify-QueryClauses

Conversation

@sergio-bobillier

Copy link
Copy Markdown
Collaborator

Simplifies the logic that ensures that the class has a single top-level clause. This simplification also gets rid of a re-assignment of the same top-level clause when #bool was called multiple times.

@julian711 julian711 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the simplification, behavior looks preserved. Two optional test-coverage suggestions for spec/jay_api/elasticsearch/query_builder/query_clauses_spec.rb (the spec isn't touched by this PR, so they're posted here rather than inline):

1. #<< when a Bool is already the top-level clause

<< now goes through the same setter, so it should raise after bool. This case isn't covered yet:

context 'when a boolean clause has already been added' do
  before { query_clauses.bool }

  it 'raises a JayAPI::Elasticsearch::QueryBuilder::Errors::QueryBuilderError' do
    expect { method_call }.to raise_error(
      JayAPI::Elasticsearch::QueryBuilder::Errors::QueryBuilderError,
      'Queries can only have one top-level query clause, to use multiple ' \
      'clauses add a compound query, for example: `bool`'
    )
  end
end

2. #bool returns the same clause when already boolean

The description mentions removing the re-assignment when #bool is called repeatedly. The "already a boolean query" context checks that Bool.new isn't called again; asserting identity would lock this in:

it 'returns the same boolean clause that was created before' do
  expect(method_call).to be(bool_clause)
end

Nit: the @return tag on the private top_level_clause= setter has no effect, since Ruby ignores a setter's return value when it's called as an assignment. In #bool, clause = if ... else self.top_level_clause = Bool.new end relies on the assignment expression's value, which is correct but easy to misread.

Simplifies the logic that ensures that the class has a single top-level
clause. This simplification also gets rid of a re-assignment of the
same top-level clause when #bool was called multiple times.
@sergio-bobillier
sergio-bobillier force-pushed the sb-simplify-QueryClauses branch from 35beb86 to 2feb493 Compare October 27, 2026 11:48
@sergio-bobillier

Copy link
Copy Markdown
Collaborator Author

Thanks for your review @julian711. I have applied your suggestions.

@sergio-bobillier
sergio-bobillier merged commit 0a39d19 into master Oct 27, 2026
2 checks passed
@sergio-bobillier
sergio-bobillier deleted the sb-simplify-QueryClauses branch October 27, 2026 11:56
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.

2 participants