Skip to content
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 collations to did you mean #2605

Merged

Conversation

barmintor
Copy link
Contributor

@epugh this is a rebase of your branch for collations, with some rudimentary tests added to ensure the existing API was satisfied and as well as your changes. Since @bess reviewed the original PR #2472 , I think there's consensus regarding the use case, but @jcoyne raised good questions about specs - there was a departure from intended behavior in the PR (and from the coverage goals we have for the project).

It would have been a bit more daunting to write the specs on the original PR, but since it was opened a spec for Blacklight::Response::SpellcheckComponent was merged to main, so now it's only a matter of adding to a couple of existing spec suites. Please let me know if you have any questions about the additional commits, but if you're satisfied that your use case is covered I'll remove the draft tag from this PR.

@epugh
Copy link
Contributor

epugh commented Jan 17, 2022

@barmintor this looks great, thank you for taking this on. Honestly, what I did was kind of a "late night make this work for a demo tomorrow" level code, so this looks great. I'll close my PR in favour of this one!

@barmintor barmintor marked this pull request as ready for review January 17, 2022 22:22
@barmintor
Copy link
Contributor Author

Awesome, thanks for getting back to me! I'll remove the draft tag.

@barmintor barmintor requested a review from bess January 17, 2022 22:25
@tpendragon tpendragon merged commit 2a1f43a into projectblacklight:main Feb 9, 2022
@epugh
Copy link
Contributor

epugh commented Feb 10, 2022

thanks for the merge @tpendragon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants