-
Notifications
You must be signed in to change notification settings - Fork 41
Create a search builder for the range limit route #325
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
base: main
Are you sure you want to change the base?
Conversation
This helps follow the pattern of having a class for each search builder rather than creating them dynamically. It may also allow Blacklight to remove the SearchBuilder#append and #except methods
ea16eaa to
63667cf
Compare
|
Hi, so right now my app has local cusotmizations it wants to apply to search builders -- say authorization stuff. In Blacklight 9.beta8, my local app applies them in it's local With this proposed change, would it also have to apply them in a locally customized RangeLimitSearchBuilder? So all users of Or am I misunderstanding? Thanks for any insight! |
|
Relevant to considering projectblacklight/blacklight#3762 , so making a link. |
Since RangeLimitSearchBuilder extends SearchBuilder, I wouldn't think you would need to make any changes. |
|
Ah, I missed that, thanks. It is a challenge though. An actual Blacklight application can have multiple CatalogController-type controllers, with different SearchBuilders. Each of which might want to use the Blacklight Range Limit. In the current architecture, blacklight_range_limit can simply say in installation instructions "make sure you include our behavior in any search builder you want to use it with", and then automatically at runtime say "take whatever the current search builder is, and do the right behavior". under htis re-architect... I guess you'd have to set up (manually or with code generation) a different RangeSearchBuilder for every controller/search builder you'd want to use it with, and then configure the relevant controller to use the right RangeSearchBuilder. That could be done, although it seems cumbersome, but I think might need some additional things not yet in this PR to make it work out... would you need/want a I see you have the specific range search builder as a param... but not sure the best way to actually architect the app to call the "correct" one there, when you have more than one catalog controller. Right now, pre-this-PR, nothing special is needed, it all Just Works, it seems like we're asking for quite a bit more code and configuration to be tracked in the local app, and more points of coupling between range limit plugin and blacklight itself. |
I'm not understanding. No more code or configuration is needed in this branch than is necessary on main. |
In an application with more than one CatalogController-type controller, with different SearchBuilders. Which is a design I think BL means to support? Sorry, I tried to explain that above, but it's hard to talk about code. The actual CI test case only tests the equivalent of a freshly generated BL app with only one CatalogController and only one SearchBuilder, so no more code is needed in the test case, true. |
|
Yes, that makes sense, if you added a second controller with a separate SearchBuilder, then you'd also want to create a separate RangeLimitSearchBuilder that extends that other SearchBuilder. |
|
Previously, there was no extra work you had to do to install To me this is an undesirable change, fairly confusing to explain/document (or write fiddly code-generation tools for), resulting in a lot of extra boilerplate code that has to all match. I remember how much work some folks put into blacklight to make it possible to have more than one controller (searching different things), it was a priority feature at one point to make possible/easy. |
This helps follow the pattern of having a class for each search builder rather than creating them dynamically. It may also allow Blacklight to remove the SearchBuilder#append and #except methods