update lucene escaping #233
Conversation
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
prasmussen15
had a problem deploying
to
development
GitHub Actions
Failure
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 3b1205c in 30 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. graphiti_core/helpers.py:60
- Draft comment:
The characters 'O', 'R', 'N', 'T', 'A', 'D' are not typically special characters in Lucene syntax and do not need escaping. Consider removing these from theescape_map. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_wOBPD7BKQ8NCLHiz
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -57,6 +57,12 @@ def lucene_sanitize(query: str) -> str: | |||
| ':': r'\:', | |||
| '\\': r'\\', | |||
| '/': r'\/', | |||
| 'O': r'\O', | |||
There was a problem hiding this comment.
The characters 'O', 'R', 'N', 'T', 'A', 'D' are not special characters in Lucene and do not need escaping. Consider removing these from the escape map.
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 8b8b9a4 in 21 minutes and 46 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tests/helpers_test.py:27
- Draft comment:
The backslash at the beginning of the string seems unnecessary and could lead to confusion. Consider removing it for clarity.
'This has every escape character \+ \- \&\& \|\| \! \( \) \{ \} \[ \] \^ \\" \~ \* \? \: \\\\ \/',
- Reason this comment was not posted:
Confidence changes required:50%
The copyright notice is present, and the code is generally well-structured. However, there is a minor issue with the escape character in the test case.
Workflow ID: wflow_6IDW8MYzWHYWOU5v
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| 'This has every secape character + - && || ! ( ) { } [ ] ^ " ~ * ? : \\ /', | ||
| 'This has every secape character \+ \- \&\& \|\| \! \( \) \{ \} \[ \] \^ \\" \~ \* \? \: \\\ \/', | ||
| 'This has every escape character + - && || ! ( ) { } [ ] ^ " ~ * ? : \\ /', | ||
| '\This has every escape character \+ \- \&\& \|\| \! \( \) \{ \} \[ \] \^ \\" \~ \* \? \: \\\ \/', |
There was a problem hiding this comment.
The expected result for the first query seems incorrect. The leading backslash is unnecessary and should be removed.
Important
Update
lucene_sanitize()to escape additional characters and adjust tests accordingly.lucene_sanitize()inhelpers.pyto escape additional characters:O,R,N,T,A,D.test_lucene_sanitize()inhelpers_test.pyto include new escape characters in test cases.This description was created by
for 8b8b9a4. It will automatically update as commits are pushed.