Fix #2 from hydrogen-launcher #10
Conversation
Link to issue: lgeiger/hydrogen-launcher#2
| return `start ${terminal} /k "${_joinCommands(cwd, command, ' & ')}"`; | ||
| // Every terminal on Windows has its own arguments. | ||
| var args = { | ||
| cmd: '/k', |
There was a problem hiding this comment.
cmd works too. But I think we should support cmd.exe as well.
There was a problem hiding this comment.
Ok. The reason I brought it up was because of the Travis config. Should we adapt it here or adapt Travis?
We can adapt it here with:
args['cmd.exe'] = args.cmd;so that we're not duplicating the arguments for what is really the same binary.
There was a problem hiding this comment.
I think we should adapt it here to allow the user to pass either cmd or cmd.exe. I don't know if we should do this for powershell and cmder as well.
|
Thanks for the PR! |
| powershell: '-NoExit' | ||
| }; | ||
| var term = terminal.toLowerCase().trim(); | ||
| return `start ${terminal} ${args[term]} "${_joinCommands(cwd, command, ' & ')}"`; |
There was a problem hiding this comment.
What happens if term isn't in args?
There was a problem hiding this comment.
Oh, I didn't think about that case. We could set a default but in most cases, it won't work because the default argument won't work in most terminals.
There was a problem hiding this comment.
I think we could just use no argument in that case.
There was a problem hiding this comment.
That'd be possible but the majority of terminals just pop up and then close immediately if you're not specifying a specific argument.
There was a problem hiding this comment.
That's true. But I think we should try it. We can always add more terminals later.
There was a problem hiding this comment.
The reason @lgeiger is suggested taking a look at when term isn't in args is that it's why the tests are currently failing. This can be remedied by adding cmd.exe (in addition to cmd) as suggested earlier or by adapting the tests.
|
Oh sorry my comment about updating the tests was wrong. |
|
I'll leave this to @lgeiger to merge and release. Thanks again for the PR and incorporating feedback @Aaronmacaron! |
|
@Aaronmacaron Thanks a lot for your work! |
|
@Aaronmacaron Would you like to join us and help maintain |
|
@lgeiger What does it mean to maintain But in general, that sounds very interesting. |
|
We bring you onto the org in nteract as a member, and if/when you have time to review pull requests or issues you bear the privilege and burden of being able to merge PRs, close issues, etc. It's mostly about making sure that we scale out as humans so that it's not just me and Lukas on this package. The other way of putting it is that we enjoyed working with you so much that we want to gift you the same responsibilities we bear. 😉 |
|
That sounds great. I'd really like to help you out maintaining those projects. |
|
Great to have you on board 🎉 |
Link to issue: lgeiger/hydrogen-launcher#2
There are a lot more Terminals on Windows and a lot of them are actually used a lot because cmd is not that popular. So there may be a need for adding more terminals.