srb-rbi with no args just prints usage (#1006) #1063
Conversation
Yeah there are regrettably no tests for any subcommand other than |
I'd be happy to add like a simple minitest unit test for this! I see that you're using minitest elsewhere. |
There was a problem hiding this comment.
Sorry for the delayed response.
I'm fine with the new feature itself, but it seems like a rather big diff for such a small feature. Is there a reason for that? Or is the diff size accidental?
Also, looks like you have some merge conflicts since this branch was opened. I'll try to review it faster next time so you don't get conflicts.
We don't want to use minitest for this. We want to test the CLI itself. I'd prefer to leave this untested, unless you build something like the existing https://github.com/sorbet/sorbet/blob/master/gems/sorbet/test/snapshot/test_one.sh#L249 See the README to see what i mean. |
525afb7 to
7c81499
Compare
|
@jez: I've rebased from master and reduced the diff size (most of that was just due to re-ordering some method definitions). I think this is ready to go! LMKWYT! |
| @@ -73,7 +102,7 @@ actions: | |||
| STDOUT.write(emojify("❔", "Would you like to continue? [Y/n] ")) | |||
| if STDIN.isatty && STDOUT.isatty | |||
| begin | |||
| input = Kernel.gets&.strip | |||
| input = STDIN.gets&.strip | |||
There was a problem hiding this comment.
Had to change because of this
There was a problem hiding this comment.
Just ran into this issue myself running srb rbi update. If this PR gets delayed I'd love to see this change as a separate PR.
There was a problem hiding this comment.
I now see there's another PR of yours addressing this as well. #1066. Thanks for your work!
|
@jez: Anything blocking this getting merged? |
|
@pje sorry for the delay. this looks great to me, thanks! i'll merge when the test pass. |
|
@pje I had to refactor some things to get the tests to pass. You can see the diff in 93bb628 . The tl;dr is that because the |
|
Thanks for the assist, @jez! 🙇 |
* Rebase from master, decrease diff size as much as possible * Fix tests
Implements #1006.
AFAICT there were no tests for
srb-rbi! So I didn't write any.