Conversation
📝 WalkthroughWalkthroughThe RQL query builder now requires a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 531-538: The _nest method contains excess blank lines violating
WPS473; remove the extra blank lines so the body is compact (declare name =
collection_name.replace("__", "."), then collection = self._to_string(self) if
self.children else self.expr or "", then expr = f"{op}({name},{collection})",
then return self.new(expr=expr)) without blank lines between these statements;
update the function _nest in class/method where Self and new(...) are used to
keep the same logic but with no extra empty lines.
- Around line 260-286: The docstring examples for the any and all methods are
incorrect; update the examples in the any() and all() docstrings to reflect the
actual RQL output format produced by _nest using OP_ANY/OP_ALL (e.g., show
gt(orderQty,11) instead of orderQty=11, and any(saleDetails,and(...)) or
all(saleDetails,and(...)) for combined queries) so they match the real output
syntax produced by RQLQuery._nest and the OP_ANY/OP_ALL operators.
In `@tests/unit/rql/query_builder/test_rql_all_any.py`:
- Around line 20-24: The test function test_all_multiple_conditions contains
blank lines inside the test body which violate AAA05; remove the empty lines so
the test has a single contiguous block (e.g., no blank lines between the
RQLQuery assignments and the final query expression) and apply the same removal
to the other affected test functions referenced (around lines 31-35) to satisfy
the linter rules.
dbb3569 to
dde678d
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 267-270: Update the docstring examples for the any() method in
rql/query_builder.py to match the actual output of _nest(): for a single
predicate use any(saleDetails,gt(orderQty,11)) and for combined predicates use
any(saleDetails,and(gt(orderQty,11),lt(price,100))). Ensure the examples
reference the any() method and reflect that the collection name is the first
argument to any() and the combined predicates are passed as the second argument
(produced by _nest()).
- Around line 283-284: The docstring example for combining multiple conditions
is wrong: update the example under RQLQuery (the usage of .all in the docstring)
so the collection name is the second argument to all() instead of inside the
and(), i.e. change the expected output from
all(and(saleDetails,gt(orderQty,11),lt(price,100))) to
all(saleDetails,and(gt(orderQty,11),lt(price,100))) so the RQLQuery/.all
documentation matches the actual output format.
dde678d to
eb284ec
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/rql/query_builder/test_rql_all_any.py (1)
30-37: Consider adding tests for OR operator.The current tests cover combining conditions with
&(AND). For completeness, consider adding tests for|(OR) operator to verify the output format likeany(saleDetails,or(gt(orderQty,1),lt(price,100))).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/rql/query_builder/test_rql_all_any.py` around lines 30 - 37, Add a new unit test mirroring test_any_multiple_conditions that constructs two RQLQuery instances (e.g., order_qty_query = RQLQuery(orderQty__gt=1) and price_query = RQLQuery(price__lt=100)), combine them with the OR operator using the pipe symbol (order_qty_query | price_query), call .any("saleDetails") on the combined query, convert to string and assert the result equals "any(saleDetails,or(gt(orderQty,1),lt(price,100)))"; this verifies the OR path in the RQLQuery and any() formatting for the or(...) form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/rql/query_builder/test_rql_all_any.py`:
- Around line 30-37: Add a new unit test mirroring test_any_multiple_conditions
that constructs two RQLQuery instances (e.g., order_qty_query =
RQLQuery(orderQty__gt=1) and price_query = RQLQuery(price__lt=100)), combine
them with the OR operator using the pipe symbol (order_qty_query | price_query),
call .any("saleDetails") on the combined query, convert to string and assert the
result equals "any(saleDetails,or(gt(orderQty,1),lt(price,100)))"; this verifies
the OR path in the RQLQuery and any() formatting for the or(...) form.



Any and all are treating nested conditions incorrectly. It wraps entire set of conditions, instead of properly using collection name:
Collection Name is used because inferring possible collections in query values is complex and prone to inference errors. Simplest and safest way I can think of is using collection name.
https://softwareone.atlassian.net/browse/MPT-18174
Closes MPT-18174