Add regex search option for text searches #107
Conversation
etanttila
force-pushed
the
regex-text-filtering
branch
from
13f5d6f
to
b14475c
Compare
|
@jsorva suggested that the checkbox label would be changed from "Use regex search" to "Regex search" (I suppose the Finnish translation would respectively be changed from "Käytä regex-hakua" to "Regex-haku"). I will do this minor change at some point unless opposing opinions arise, but this can sill be reviewed before that. |
| msgid "Not a valid regex pattern" | ||
| msgstr "Ei ole säännöllinen lauseke" |
There was a problem hiding this comment.
Would "Ei säännöllinen lauseke" be better than "Ei ole säännöllinen lauseke"?
feedback/filters.py
Outdated
| """ | ||
| template_name = "feedback/widgets/combo_textsearch_widget.html" | ||
|
|
||
| def __init__(self, attrs=None): |
There was a problem hiding this comment.
__init__ should have a return type annotated with -> None:
https://peps.python.org/pep-0484/#the-meaning-of-annotations
feedback/filters.py
Outdated
| class ComboTextSearchField(forms.MultiValueField): | ||
| widget = ComboTextSearchWidget | ||
|
|
||
| def __init__(self, *args, **kwargs): |
There was a problem hiding this comment.
__init__ should have a return type annotated with -> None:
https://peps.python.org/pep-0484/#the-meaning-of-annotations
feedback/filters.py
Outdated
| } | ||
| super().__init__(widgets, attrs) | ||
|
|
||
| def decompress(self, value) -> tuple[str, bool]: |
There was a problem hiding this comment.
Should the parameter value have a type annotation?
There was a problem hiding this comment.
A very good question. It can either be None or a tuple[str, bool], so if a type annotation is added, should it be Optional[tuple[str, bool]]?
Also, should the return value type annotation be actually tuple[Optional[str], Optional[bool]] (or Union[tuple[str, bool], tuple[None, None]], since the return value can either be tuple[str, bool] or (None, None)?
There was a problem hiding this comment.
(I also realized that it probably would be a good idea to change this method slightly to check if value rather than if value is None (and respectively swap the return values) (so if somehow the empty value is something else than None, it still would work).
There was a problem hiding this comment.
A very good question. It can either be
Noneor atuple[str, bool]
Although I guess it could also be a list consisting of a str and bool, but list only accepts one type argument, so that cannot be provided as a type hint. I guess I just have to stick with the Optional[tuple[str, bool]]
feedback/filters.py
Outdated
| """ | ||
| field_class = ComboTextSearchField | ||
|
|
||
| def __init__(self, *args, lookup_expr=None, **kwargs): |
There was a problem hiding this comment.
__init__ should have a return type annotated with -> None:
https://peps.python.org/pep-0484/#the-meaning-of-annotations
Allow user to select to use regex search for student form contents and teacher response. This is done by checking a checkbox by the field. Implement filter, field and widget that allow selecting between default search method and a regex search. Implement simple validation and error handling (also for exercise identifier, which didn't validate regex) for regex. Change form styling to use flex, so the checkbox and label can also be positioned well. Fix apluslms#87
etanttila
force-pushed
the
regex-text-filtering
branch
from
5205471
to
90a51aa
Compare
Description
What?
Allow user to select to use regex search for student form contents and teacher response. This is done by checking a checkbox by the field.
Add also simple validation and error styling (because the default error message appearance breaks layout):
The layout and styling should work well responsively on different sizes of screens, e.g.:
Why?
Juha Sorva wants support for regex filtering of form contents.
How?
Implement filter, field and widget that allow selecting between default search method and a regex search.
Implement simple validation and error handling (also for exercise identifier, which didn't validate regex) for regex.
Change form styling to use flex, so the checkbox and label can also be positioned well.
Fixes #87
Testing
Remember to add or update unit tests for new features and changes.
What type of test did you run?
Tested that the normal text search still seems to work, as well as the new regex search. (And that searching works also when the text field is empty). Tested also that there is a error message if the regex search is used but the search string isn't valid regex.
Did you test the changes in
Think of what is affected by these changes and could become broken
Translation
Programming style
Have you updated the README or other relevant documentation?
Added some documentation strings withing the code.
Perhaps Aplus manual should be updated also towards the end of the summer to provide info about new changes (e.g. this filtering, importing feedback tags, and other possible changes).
Is it Done?
Clean up your git commit history before submitting the pull request!