Apply the double-search change to simple search #208
Conversation
...and also add some comments for visibility.
| @@ -324,6 +326,8 @@ function simple_search($params) | |||
|
|
|||
| //***** finalize query parts *****// | |||
|
|
|||
| $sql .= ") SELECT *, COUNT(*) OVER() as full_count FROM results"; | |||
There was a problem hiding this comment.
So hold on, in https://github.com/LibriVox/librivox-catalog/pull/207/files#diff-cb39737e00dff1ebcb3d8fc914228975e384415679c2f9495c369c537b65318cR183 you changed the code to use the full_count directly in the search results. In that same PR, you modified advanced_title_search() to return the full count, but you didn't touch simple_search(). You're making simple_search() return the full_count in this PR, so my question is - how does simple search even work currently now? It seems to work, but I'm not sure I understand how.
There was a problem hiding this comment.
Since that first PR was merged, simple search does not work on current master. It wants that column, but doesn't get it.
Since it is working in prod right now, it must still be running a version before that PR.
There was a problem hiding this comment.
I git pulled in prod when I merged that PR. Very curious...
There was a problem hiding this comment.
I just tested again: current master still fails on simplesearch, giving an HTML error message rather than the JSON array the page expects.
I don't know how that git pull could have failed, but I'm glad it didn't take, until this one is in. We did say simplesearch needed patched. 😬
There was a problem hiding this comment.
I just tested again: current master still fails on simplesearch, giving an HTML error message rather than the JSON array the page expects.
Can you link an example? I'm legit trying every search that I know of, and they all appear to be working.
I don't know how that
git pullcould have failed, but I'm glad it didn't take, until this one is in. We did say simplesearch needed patched. 😬
You did, and I think this is a case of my day job expectations and assumptions leaking into LibriVox. In my day job, master is always working, and we never knowingly merge broken or incomplete code. In my mind this is a shared understanding that everyone else, including yourself, has as well, in all projects. That's obviously not the case.
I would strongly prefer if we made such a policy explicit for LibriVox: never knowingly merge broken or incomplete code, even if we don't have the CI coverage needed to automatically enforce that. But this is something that we can discuss.
There was a problem hiding this comment.
Edit: In case you saw the earlier version, this has been edited. For some reason, it's not showing the 'Edited' tag.
Can you link an example? I'm legit trying every search that I know of, and they all appear to be working.
That would seem to be because master isn't what's in prod. With my dev instance on librivox/master, I get nothing but a spinning wheel.
In my day job, master is always working, and we never knowingly merge broken or incomplete code.
I share the same expectation: nothing that isn't working should actually make it in to master. I just didn't think I was merging the incomplete code. I think I see the disconnect.
The PR was tagged with "[Sanity Check]", which you recognized as meaning that it needed some feedback. I should have also been more explicit (as Gareth was in his comment) that it was not complete even if it was the right technical direction.
When you asked if I had "any objections to merging this", I thought you referred to the PR in general, which I thought we all knew was not complete and needed a second commit. So I said:
if the technical direction is sound, then no, I'm not opposed. I just have to patch the simplesearch part too. I'll get a second commit ready soon!
Perhaps I should have instead said:
if the technical direction is sound, then I'll get simplesearch fixed per Gareth's comment. I'll remove the "[Sanity Check]" tag when I've got it all ready and tested. I expect to have the second commit ready soon!
We can also talk through email if you prefer.
There was a problem hiding this comment.
Edit: In case you saw the earlier version, this has been edited. For some reason, it's not showing the 'Edited' tag.
Can you link an example? I'm legit trying every search that I know of, and they all appear to be working.
That would seem to be because master isn't what's in prod. With my dev instance on librivox/master, I get nothing but a spinning wheel.
Prod is definitely on latest master, hence my asking for an example or URL where I can hopefully see simple search failing, if only on staging or localdev.
In my day job, master is always working, and we never knowingly merge broken or incomplete code.
I share the same expectation: nothing that isn't working should actually make it in to master. I just didn't think I was merging the incomplete code. I think I see the disconnect. The PR was tagged with "[Sanity Check]", which you recognized as meaning that it needed some feedback. I should have also been more explicit (as Gareth was in his comment) that it was not complete even if it was the right technical direction.
When you asked if I had "any objections to merging this", I thought you referred to the PR in general, which I thought we all knew was not complete and needed a second commit. So I said:
if the technical direction is sound, then no, I'm not opposed. I just have to patch the simplesearch part too. I'll get a second commit ready soon!
Perhaps I should have instead said:
if the technical direction is sound, then I'll get simplesearch fixed per Gareth's comment. I'll remove the "[Sanity Check]" tag when I've got it all ready and tested. I expect to have the second commit ready soon!
I'm used to the "[WIP]" (Work in Progress) or "[DNM]" (Do Not Merge) tags in the commit message title. DNM is much stronger and indicates trying to debug something, often with the CI system itself. The expected outcome of a DNM patch is for it to be abandoned. WIP indicates that the patch isn't ready to merge yet, but there's an expectation that the WIP tag will get removed, at which point the patch will be ready to merge. In retrospect, PR #207 should have been a [WIP].
In my day job, because we use Gerrit and a gating CI, there's also the handy vote system, so the patch author can vote "Workflow -1" on their own patch, clearly indicating that they don't want it to merge at this time. We can try something similar here, by requiring approval from yourself (@redrun45) and myself on all patches (and potentially @garethsime if they're willing to accept the responsibility?). That way, if you're proposing a PR that isn't ready yet, you can disapprove your own PR, and then change your approval once it's ready.
|
OK, I think it had to do with a config difference between prod and staging/localdev. By enabling error logging ( When I merged and pulled this PR, those stopped. |
I see! What I was seeing on localdev is that the page never switched to a new URL - it just stayed on whatever page you started the search from, which was why I couldn't just give an example. With logging on, I suppose the server goes ahead and sends the AJAX response, instead of the HTML error message I was seeing.
Sorry to put you to the extra trouble, but this sounds good. I don't do anything like this at my day job, so having some guard rails that force me to be 100% clear and matching your expectations sounds like the way to go. |
...and also add some comments for visibility.