isso
isso copied to clipboard
isso-comments
server: Set reply sorting to 'oldest'
Checklist
- [x] All new and existing tests are passing
- [ ] (If adding features:) I have added tests to cover my changes
- [x] (If docs changes needed:) I have updated the documentation accordingly.
- [x] I have added an entry to
CHANGES.rstbecause this is a user-facing change or an important bugfix - [x] I have written proper commit message(s)
What changes does this Pull Request introduce?
Quick SQL patch to fix #1026
Why is this necessary?
Current reply sorting leads to confusion. See linked issue for details.
NOTE: I tested this briefly and it seems to be working (and of course unit tests pass) but I wasn't sure if the sorting option was actually going through. Hard to tell on the demo page. Will probably test tmrw over curl.
Just confirmed the patch to be working now - you can see the results on my site. Here's the comment I showed as an example last time:
Thank you for addressing the feedback. Please squash your commits and give the resulting commit a more descriptive title/body, such as db: Set reply sorting to 'oldest'.
You also checked in an updated package-lock.json to your commit when you didn't mean to, please remove that as well.
Sorry for leaving this in limbo for so long. The test suite (predictably) fails for me:
FAILED isso/tests/test_comments.py::TestComments::testFeed - AssertionError: '"1-1"' != '"1-2"'
FAILED isso/tests/test_comments.py::TestComments::testGetSortedByNewestWithNested - AssertionError: Lists differ: [2, 3, 4, 5, 6] != [6, 5, 4, 3, 2]
FAILED isso/tests/test_comments.py::TestComments::testGetSortedByUpvotesWithNested - AssertionError: Lists differ: [2, 3, 4, 5, 6] != [6, 3, 2, 4, 5]
@ggtylerr would you mind updating the tests to reflect the new sorting? Thank you!
Hi, sorry, I didn't get any notification for this when ix5 replied for some reason. I'll update it whenever I'm off my shift today.
I fetched upstream, cleared the merge conflict and updated the tests. Everything seems to work when I run it with nektos act, although I can't run isso itself since pip isn't letting me pull misaka.
One thing to note is that testGetSortedByNewestWithNested and testGetSortedByUpvotesWithNested now have the same order, due to them testing for the order of replies and not comments + replies. I consider this fine and these two still test whether or not the reply order is modified, but it's still something to note. Might need to modify to test that, or just have a separate test.
(Also I cannot for the life of me figure out how to squash nicely w/ the merge commit, I'm sorry :cry:)
LGTM. I'll wait a few days to give @ix5 a chance to object. I'll merge otherwise.
We should do another release at some point; first we should fix the failing tests though.
Sorry for the late reply, updated PR now LGTM.
@ggtylerr you can do a rebase including a squash of superfluous merge/editing commits, that's the way I'd recommend at least.
@ggtylerr you can do a rebase including a squash of superfluous merge/editing commits, that's the way I'd recommend at least.
It's uh, already merged though 😅 I probably should've said to just use the squash + merge option in GH before it got merged, but it's whatever
It's uh, already merged though 😅 I probably should've said to just use the squash + merge option in GH before it got merged, but it's whatever
No worries, just format your updated commit history correctly next time. git rebase -i is a big help there.