-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add a non-whitespace pattern to SearchQueriesParam. #74
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
+ Coverage 95.36% 95.93% +0.57%
==========================================
Files 26 26
Lines 2609 2609
==========================================
+ Hits 2488 2503 +15
+ Misses 121 106 -15
|
Wouldn’t this prevent multi-word queries, like searching for “foo bar”? |
Hmm, apparently it tries to match the entire value even without ^$ |
@@ -18,6 +18,7 @@ class SearchQueriesParam(BaseModel): | |||
description="Input 1 search query per line (e.g. foo bar).", | |||
json_schema_extra={ | |||
"widget": "textarea", | |||
"pattern": r"(.|\r?\n)*\S+(.|\r?\n)*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When defining the regular expressions, it's important to note that the string is considered valid if the expression matches anywhere within the string.
https://json-schema.org/understanding-json-schema/reference/string#regexp
So, I’m thinking this should do the trick:
"pattern": r"(.|\r?\n)*\S+(.|\r?\n)*", | |
"pattern": r"(.|\r?\n)*?\S", |
Or maybe even (your original?):
"pattern": r"(.|\r?\n)*\S+(.|\r?\n)*", | |
"pattern": r"\S", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the initial one didn't work on a multi-word input (neither does r"\S"
) so I don't know if the quote is true. Or maybe the frontend code uses it incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe the frontend code uses it incorrectly.
Oh, right, the frontend is most likely not using some JSON schema library, and instead uses its own implementation, which might be assuming an initial ^
(like Python’s re.match
vs re.search
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is not super straightforward - would it be possible to add a test for it? I.e. check that the validation passes with input A, and fails with input B?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does \S
pass those tests?
No description provided.