mostly my suggestions will be minor refactors at best … post it as a pull request
I’m happy to look at a PR, but I think I’m unlikely to merge one that’s minor refactors: I’ve evaluated the current code through manual testing, and if I were going to make changes to it I’d need another round of manual testing to verify it still worked. Which isn’t that much work, but the benefit is also small.
One general suggestion I have is to write some test code that can notify you when something breaks
It’s reasonably fast for me to evaluate it manually: pick a post that should have some comments (including nested ones) and verify that it does in fact gather them. Each time it runs it tells me the number of comments it found (via the title bar) and this is usually enough for me to tell if it is working.
I think this is an unusually poor fit for automated tests? I don’t need to keep the code functional while people other than the original author work on it, writing tests won’t keep dependencies from breaking it, the operation is simple enough for manual evaluation, the stakes are low, and it’s quite hard to make a realistic test environment.
I really don’t like the no-semicolons JS style. I’ve seen the arguments that it’s more elegant, but a combination of “it looks wrong” and “you can get very surprising bugs in cases where the insertion algorithm doesn’t quite match our intuitions” is too much.
What’s the advantage of making alreadyClicked a set instead of keeping it as a property of the things it’s clicking on?
In this case I’m not at all worried about memory leaks, since the tab will only exist for a couple seconds.
The getExpandableComments simplification is nice!
I haven’t tested it, but I think your collectComments has a bug in it where it will include replies as if they are top level comments in addition to including them as replies to the appropriate top level comments.
I’m happy to look at a PR, but I think I’m unlikely to merge one that’s minor refactors: I’ve evaluated the current code through manual testing, and if I were going to make changes to it I’d need another round of manual testing to verify it still worked. Which isn’t that much work, but the benefit is also small.
It’s reasonably fast for me to evaluate it manually: pick a post that should have some comments (including nested ones) and verify that it does in fact gather them. Each time it runs it tells me the number of comments it found (via the title bar) and this is usually enough for me to tell if it is working.
I think this is an unusually poor fit for automated tests? I don’t need to keep the code functional while people other than the original author work on it, writing tests won’t keep dependencies from breaking it, the operation is simple enough for manual evaluation, the stakes are low, and it’s quite hard to make a realistic test environment.
I totally got carried away. 😅
[Here’s what I did](https://github.com/jeffkaufman/comments-selenium/pull/1). I don’t even know if it is a real suggestion anymore. Maybe you’ll find inspiration from some of it. Maybe not.
Interesting to read through! Thoughts:
I really don’t like the no-semicolons JS style. I’ve seen the arguments that it’s more elegant, but a combination of “it looks wrong” and “you can get very surprising bugs in cases where the insertion algorithm doesn’t quite match our intuitions” is too much.
What’s the advantage of making
alreadyClickeda set instead of keeping it as a property of the things it’s clicking on?In this case I’m not at all worried about memory leaks, since the tab will only exist for a couple seconds.
The
getExpandableCommentssimplification is nice!I haven’t tested it, but I think your
collectCommentshas a bug in it where it will include replies as if they are top level comments in addition to including them as replies to the appropriate top level comments.