Clean up SearchQuery API (less misleading names, fluent interface) #64
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SearchQuery->search() is highly misleading - it implies that a search is executed, while in fact it only adds a search term to an internal property. This gets even more confusing since the actual method executing passing on the search request to the backend is also called SearchIndex->search(). Renamed other methods along the same lines, for example start() is a confusing name for adding a start value for pagination, and inClass() implies a boolean return while in fact its adding a filter. Also added getters to get naming a bit more consistent (e.g. filter() sets $require). Set the deprecation notices to 2.0, so a future release.
| return $this->limit; | ||
| } | ||
|
|
||
| function setPageSize($page) { |
There was a problem hiding this comment.
I think these methods need public and some phpdoc. I'm not sure what this is doing immediately by reading the code.
|
@chillu - do you have any desire to revisit this and push it over the line? |
|
@chillu as part of the ORB work, I'm considering integrating equivalent changes as a way of reducing developer confusion when working with the module. If you're happy for me to lift and tweak I will do so 😄 |
Yes please! Thank you Andrew :) |
|
@chillu can you give contributors push access to your PR branch, please :) |
|
I've rebased this, just need to be able to push it |
|
oh, looks like @andrewandante has already done it in #214 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SearchQuery->search()is highly misleading - it implies that a search is executed,while in fact it only adds a search term to an internal property.
This gets even more confusing since the actual method executing passing
on the search request to the backend is also called
SearchIndex->search().And many implementations also create a
Page_Controller->search()one layer further up.Renamed other methods along the same lines, for example
start()is a confusingname for adding a start value for pagination, and
inClass()implies a booleanreturn while in fact its adding a filter.
Also added getters to get naming a bit more consistent (e.g.
filter()sets$require).Set the deprecation notices to 2.0, so a future release.