dev: Add more tests for parseCmd #90
Conversation
jdkaplan
force-pushed
the
jdk-parser-tests
branch
2 times, most recently
from
17c3a79 to
bbc1605
Compare
| "get-reviews 0": {"get-reviews", []string{"0"}}, | ||
| "get-reviews 1": {"get-reviews", []string{"1"}}, | ||
| "get-reviews 5": {"get-reviews", []string{"5"}}, | ||
| "get-reviews 10": {"get-reviews", []string{"10"}}, |
There was a problem hiding this comment.
I want to argue that get-reviews 0 is a bug, but it has funny output, so I'm keeping it 😄
| var rejectedCommands = []string{ | ||
| "", | ||
|
|
||
| // Funnily enough, these *do* give you what you want! |
There was a problem hiding this comment.
Is there another TODO task here, "make help match generously"? On any string with help in it (that isn't add-review.*),
It seems like you're asking for help -- here's how to use Pairing Bot:
There was a problem hiding this comment.
From a functionality perspective, the user does still get the help text! Well, they should, but I broke that here by being over-eager on error returns. I'll stack another commit on here to fix that.
I could see the default-to-help error path having a nicer version of the message that starts with your suggestion! I'll sneak it into the next PR where I refactor the parser itself to be infallible.
|
OK hi! I left a bunch of nitpicky comments which you should feel free to ignore -- they're all strictly about Golang style. The semantics of the change are great and helpful -- thank you! For background, I'm used to doing three kinds of code review:
I don't care about enforcing the Golang style guide on this project, and I would never want it to block a contribution. But I do see where sticking with cross-project idioms can help people onboard -- if not to Pairing Bot, into other Golang environments. (I'm sure using So, feedback-on-feedback: Are these (kinds of) comments useful? Or not worth the time on either end? |
There was a problem hiding this comment.
We seem to have settled on very similar code review conventions, somehow! This review felt very familiar 😄
So, feedback-on-feedback: Are these (kinds of) comments useful? Or not worth the time on either end?
Yes, very worth it! In particular, I think that we have overlapping-but-slightly-different Go experiences, so I definitely want to know what stands out to you as you're reading my code.
Especially since you make the non-blocking comments truly non-blocking by approving the PR anyway! (not that blocking applies to me as an admin, but you get the idea)
| var rejectedCommands = []string{ | ||
| "", | ||
|
|
||
| // Funnily enough, these *do* give you what you want! |
There was a problem hiding this comment.
From a functionality perspective, the user does still get the help text! Well, they should, but I broke that here by being over-eager on error returns. I'll stack another commit on here to fix that.
I could see the default-to-help error path having a nicer version of the message that starts with your suggestion! I'll sneak it into the next PR where I refactor the parser itself to be infallible.
jdkaplan
force-pushed
the
jdk-parser-tests
branch
from
bbc1605 to
e1e6f61
Compare
I wanted to understand the parser tests better, so I collected them into good/bad groups and added some more cases that weren't tested before.