MPT-18065 - Fix backward incompatible issues related to RQL: Support of property comparison#213
MPT-18065 - Fix backward incompatible issues related to RQL: Support of property comparison#213albertsola wants to merge 1 commit intomainfrom
Conversation
ea23e8e to
7ba0797
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@mpt_api_client/rql/__init__.py`:
- Line 3: The __all__ export list in mpt_api_client/rql/__init__.py is unsorted
and contains an invalid Ruff noqa directive; update the __all__ declaration to
be alphabetically sorted (i.e., list "Literal" before "RQLQuery") and remove the
invalid "# noqa: WPS410" comment so the file exports the symbols Literal and
RQLQuery correctly and no longer references a non-Ruff rule.
In `@mpt_api_client/rql/query_builder.py`:
- Around line 79-83: Remove the stray blank line in the value-serialization
block so the if isinstance(value, dt.date | dt.datetime): branch and the
subsequent return str(value) are formatted per project style; locate the block
in query_builder.py that checks isinstance(value, dt.date | dt.datetime) (the
value variable) and collapse the extra blank line, then run `ruff format .` to
apply the formatter.
- Around line 9-14: The Literal class lacks type annotations; update it to be
mypy-strict compliant by annotating the attribute and methods: add a class
attribute annotation for value (e.g., value: str), change __init__ signature to
def __init__(self, value: str) -> None and set self.value = value, and annotate
__str__ to def __str__(self) -> str; ensure all uses of Literal.value and its
methods match these types.
In `@tests/unit/rql/query_builder/test_properties_values.py`:
- Around line 22-25: The file has an extra blank line between the tests
test_ne_quoted and test_ne_property causing Ruff to reformat; open the test file
and remove one of the blank lines so there are only two consecutive newlines
between the end of test_ne_quoted and the def line for test_ne_property, or
simply run the formatter (e.g., ruff format .) to apply the standard Ruff
formatting rules and commit the resulting change.
7ba0797 to
5f310e6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mpt_api_client/rql/query_builder.py (1)
21-21: Remove the invalid# noqa: WPS221.Ruff flags this as an unknown/unused noqa; the line looks short enough to drop the directive.
As per coding guidelines, "Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections."🧹 Proposed fix
-QueryValue = str | bool | dt.date | dt.datetime | Numeric | Literal # noqa: WPS221 +QueryValue = str | bool | dt.date | dt.datetime | Numeric | Literal
7234207 to
5b517cd
Compare
5b517cd to
6529d49
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 137-142: The code path that handles list operators (when op in
constants.LIST) currently builds the list literal using str(el), so string
elements are not quoted consistently; change the branch that returns
",".join(str(el) for el in value) to join the per-element output of
query_value_str (e.g., ",".join(query_value_str(el) for el in value)) so each
element is encoded with the same literal-quoting logic; update the branch
referencing op, constants.LIST, and query_value_str accordingly and ensure it
accepts list|tuple|set values.
- Line 9: The type alias QueryValue in query_builder.py currently has an invalid
inline ignore ("# noqa: WPS221"); remove that trailing comment so the line reads
without any noqa directive. Locate the definition "QueryValue = str | bool |
dt.date | dt.datetime | Numeric" and delete the "# noqa: WPS221" token so Ruff
no longer reports the unknown rule code.
---
Duplicate comments:
In `@mpt_api_client/rql/__init__.py`:
- Around line 1-3: Remove the invalid wemake `# noqa: WPS410` comment in the
module where you export Property, RQLQuery, and Value (the __all__ declaration
in mpt_api_client.rql.__init__.py); either delete the `# noqa: WPS410` suffix
entirely or replace it with a proper Ruff per-file ignore directive for the
corresponding Ruff codes (e.g., add a `# ruff: noqa: RUF100,RUF102` at the top
of the file) so Ruff no longer flags an invalid rule code while keeping the
__all__ export intact.
| return self.value | ||
|
|
||
|
|
||
| class Value: |
There was a problem hiding this comment.
I think Value is premature optimization at this stage, it does nothing. The default behavior should assume the value is not a property and just quote it. In case the value is a property, which was uncommon so far, we were not using it, user would use Property.
There was a problem hiding this comment.
Value converts different types of values: datetime, date, str, boolean, to the right string format.
Some values needs to add the quotes other not so we encapsulate that logic in the str function of Value.
Moved this out of query_value_str so we simplify and encapsulate that method.
It helps keeping things clean.
2a09408 to
4304a52
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpt_api_client/rql/query_builder.py (1)
137-142: Redundant condition check and potential list element quoting inconsistency.Line 139 is unreachable for its intended purpose: if
op in constants.COMP, thenop not in constants.LISTis already true, so line 137 would have already returned. The condition at line 139 will only trigger ifopis neither inLISTnor a valid operator (which would then fall through to the TypeError).Additionally, a past review flagged that list elements at line 142 use
str(el)instead ofquery_value_str(el), which means string elements in lists won't be quoted. However, test expectations (e.g.,in(asset.id,(first,second))) suggest this is intentional. Please confirm whether list element quoting is deliberately excluded for backward compatibility.♻️ Proposed simplification (remove redundant condition)
def rql_encode(op: str, value: Any) -> str: ... if op not in constants.LIST and isinstance(value, QueryValue | Value | Property): return query_value_str(value) - if op in constants.COMP and isinstance(value, QueryValue | Value | Property): - return query_value_str(value) if op in constants.LIST and isinstance(value, list | tuple | set): return ",".join(str(el) for el in value) raise TypeError(f"the `{op}` operator doesn't support the {type(value)} type."),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/rql/query_builder.py` around lines 137 - 142, Remove the redundant condition checking "if op in constants.COMP" because the preceding "if op not in constants.LIST and isinstance(...)" already handles COMP; delete the second branch that duplicates that logic (referencing constants.LIST/constants.COMP and the QueryValue/Value/Property checks and query_value_str). For the list branch that currently uses ",".join(str(el) for el in value), decide whether list elements should be serialized with query_value_str(el) (to ensure strings are quoted) or left as str(el) for backward compatibility; implement the chosen behavior and add a short inline comment explaining the decision near the list serialization in query_builder (and adjust tests if switching to query_value_str).
🤖 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 26-27: The __init__ method of the Value class is missing an
explicit return type annotation; update the signature of Value.__init__ to
include "-> None" (i.e., change def __init__(self, value: QueryValue) to def
__init__(self, value: QueryValue) -> None) so it matches mypy strict
requirements and is consistent with the Property class.
- Around line 15-16: The __init__ method of the Property class is missing an
explicit return type for mypy strict mode; update the signature of
Property.__init__ to include the return annotation "-> None" so it reads like a
standard initializer with an explicit None return type (locate the __init__
method inside the Property class in query_builder.py and add the annotation).
---
Nitpick comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 137-142: Remove the redundant condition checking "if op in
constants.COMP" because the preceding "if op not in constants.LIST and
isinstance(...)" already handles COMP; delete the second branch that duplicates
that logic (referencing constants.LIST/constants.COMP and the
QueryValue/Value/Property checks and query_value_str). For the list branch that
currently uses ",".join(str(el) for el in value), decide whether list elements
should be serialized with query_value_str(el) (to ensure strings are quoted) or
left as str(el) for backward compatibility; implement the chosen behavior and
add a short inline comment explaining the decision near the list serialization
in query_builder (and adjust tests if switching to query_value_str).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 137-140: The second conditional block checking "if op in
constants.COMP and isinstance(value, QueryValue | Value | Property): return
query_value_str(value)" is dead code because COMP and LIST are disjoint and the
prior "if op not in constants.LIST and isinstance(...)" already returns for all
COMP ops; remove the redundant if block (delete the lines referencing
constants.COMP and the duplicate isinstance check) so the code relies on the
initial check and the call to query_value_str(value) only once, preserving
existing behavior with constants.LIST, constants.COMP, QueryValue, Value,
Property, and query_value_str.
---
Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 15-16: The Property.__init__ constructor is missing an explicit
return type annotation required by strict mypy; update the method signature of
Property.__init__ to include "-> None" (i.e., def __init__(self, value: str) ->
None:) so the constructor is properly typed.
- Around line 26-27: The __init__ method of the Value class is missing an
explicit return type annotation; update the signature of Value.__init__ (the
constructor that takes value: QueryValue) to include the -> None return
annotation to satisfy mypy strict rules, e.g., change def __init__(self, value:
QueryValue): to def __init__(self, value: QueryValue) -> None:, leaving the body
(self.value = value) unchanged.
- Around line 141-142: The list-serialization branch in query_builder (the if
checking "op in constants.LIST" and "isinstance(value, list | tuple | set)")
uses str(el) which leaves string elements unquoted and inconsistent with scalar
handling; change the element formatting to call query_value_str(el) for each
element (i.e., replace str(el) with query_value_str(el) in the join) so list
members are quoted the same way as single values, preserving current isinstance
checks and the op in constants.LIST condition.
53b8308 to
47930f4
Compare
…ort of property comparison
47930f4 to
bb25638
Compare
|
|
@coderabbitai which issues do I missed to resolve? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 674 🏁 Script executed: Length of output: 1082 🏁 Script executed: Length of output: 0
However, there are 2 unresolved review threads from other reviewers:
Summary: You haven't missed any of my review comments—they're all addressed! The remaining items are from |
|
@coderabbitai if I have not missed anything why are you still requesting changes? should you update to approve the changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1976
[approve] |
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |



Closes MPT-18065