-
Notifications
You must be signed in to change notification settings - Fork 47
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
Issues #320: move all our regex logic into a unit tested class #563
Conversation
e6f5214
to
ea8c5cc
Compare
* | ||
* Allows searching for data in our repositories using a fluent interface. | ||
* Currently, only the regex part (definition of the search) is implemented. | ||
* ex: |
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.
Trivia: I noticed it while adding the suggestion API, is it a French abbreviation? Not sure how common it is in English compared to e.g.
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.
I can change it to e.g.
It definitely looks good to me. Only one possible concern: we end up calling the updateRegex method a scary amount of times, often to not change anything (I realized it while logging the regex). The only alternative I can think of is to remove the call from set methods and call it explicitly, only once, at the end
The only downside is remembering that we need to call updateRegex at the end. |
->setRegexWholeWords($check['whole_word']) | ||
->setRegexCase($check['case_sensitive']) | ||
->setRegexPerfectMatch($check['perfect_match']) | ||
; |
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.
I find the semicolon on a new line quite strange
I think it's more risky to have to manually add updateRegex() everytime we change a setter than to have it called automatically. Furthermore, it will not reduce the number of calls significantly, only a handful of times, because it is called in two places only, at the initial definition of the regex (we would save like 3 calls to updateRegex()) and in loops when we have to search for each word, in that case there would be no saving at all because we would have to add a call to updateRegex() anyway if it's not called automatically. |
I agree on the risk, the number of request would be always 1/3. It doesn't seem much, but it's 3+3*character typed slowly in the search bar+3 for the resulting page: that's 10 vs 30 for "window". Still, probably not worth the risk of thinking the regex is updated while it's actually not. |
7c74883
to
87d3405
Compare
Issues #320: move all our regex logic into a unit tested class
I only put in this branch the treatment of the search query.
Currently the Search class only implements methods to create the regex necesary for our various views.
This class has unit tests, so the main benefits of this patch are that the definition of the regex in models is more intuitive (creation of an object with a fluent interface) and that it should be much harder to regress our regex code in our models now.