-
Notifications
You must be signed in to change notification settings - Fork 348
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 sort option to AbstractSearchObject recommendations. #3673
Add sort option to AbstractSearchObject recommendations. #3673
Conversation
While working on this, I also thought it might be useful to build a trait for parsing configurations which could add named argument support similar to PHP's... that would make some configurations easier and more readable. But that's a project for another day! |
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 like adding the sort option where it works, and I like the documentation simplification in theory, but I'm not sure if it may add some confusion about the details.
config/vufind/searches.ini
Outdated
; EPFResultsDeferred:[GET parameter]:[result limit]:[custom heading] | ||
; EPFResults:[GET parameter]:[result limit]:[custom heading]:[filter ini section]:[sort] | ||
; Display EPF (EBSCO Publication Finder) search results. See CatalogResults above | ||
; for parameter definitions. Note that some may not apply to this search backend. |
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 like the idea of slimming down the descriptions, but I think we should be clear about which backends support which options, especially with the EBSCO ones where it is so confusing to begin with. We could still say "See CatalogResults above for parameter definitions" but then say "XYZ are not supported on this backend" or something like that. I.e. for sort, it is definitely not supported (yet anyway) for the LibGuides recommendations.
And for the [filter ini section] behavior, I didn't design it with EDS and EPF in mind, and so I didn't say in searches.ini that they supported it, but I'm not actually sure either way.
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've tested the EDS, EPF and Website modules and adjusted the documentation to reflect how things work. I can't test Summon but strongly suspect that my documentation there is correct. I also updated the LibGuides entries to indicate which things won't have any effect. Hopefully that makes this all more clear, though I'm open to further adjustments if you have suggestions.
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.
Looks great now, and the comments also provide a pretty clear enhancement path if we feel the need.
I thought it might be useful to add the ability to sort search-based recommendations -- e.g. to display "new items" with a
first_indexed desc
sort. I also simplified the comments in searches.ini by making all of the AbstractSearchObject subclasses refer back to CatalogResults for parameter definitions, to reduce the amount of duplicate documentation.