Adding in port selection to serialport-term. #1448
Conversation
This makes the terminal command much more useable when you need to open it multiple times in a row; especially when the names of the serialports are rapidly changing.
| @@ -62,6 +62,7 @@ | |||
| "nan": "^2.6.2", | |||
| "prebuild-install": "^2.4.1", | |||
| "promirepl": "^1.0.1", | |||
| "prompt-list": "^3.1.2", | |||
There was a problem hiding this comment.
This change does require a new dependency. I don't know if this will be contentious or not. Please let me know.
| @@ -99,6 +100,7 @@ | |||
| "lint": "eslint lib test bin examples", | |||
| "rebuild-all": "npm rebuild && node-gyp rebuild", | |||
| "repl": "node bin/repl.js", | |||
| "terminal": "node bin/terminal.js", | |||
There was a problem hiding this comment.
This is useful for development because of: npm/npm#7137
|
|
||
| function makeNumber(input) { | ||
| return Number(input); | ||
| } | ||
|
|
||
| args | ||
| .version(version) | ||
| .usage('-p <port> [options]') |
There was a problem hiding this comment.
Since port is no longer a required argument we can remove references to it.
bin/terminal.js
Outdated
|
|
||
| portSelection.run() | ||
| .then(answer => { | ||
| const choice = answer.split('\t')[1]; |
There was a problem hiding this comment.
This is probably the hackiest part of the whole PR. Relying on the choices to be tab separated in order to extract the comName.
I would prefer it if the prompt-list library had a name and value fields so that what you display could be different to what gets returned as an 'answer' but, alas, it does not look like that is the case.
bin/terminal.js
Outdated
| }) | ||
| .catch(error => { | ||
| console.log(`Could not select a port: ${error}`); | ||
| process.exit(-2); |
There was a problem hiding this comment.
Not sure what your policy is on the error code numbering that should be returned. Could use feedback.
There was a problem hiding this comment.
Lets make it a positive int for fewer surprises cross platform
|
My go-to is listr for this kind of thing. Dunno how it compares. Error
codes for *nix should be small positive ints, both work on Windows so I'd
go that direction.
On Sat, Jan 20, 2018, 11:26 PM Francis Gulotta <wizard@roborooter.com>
wrote:
… I'm traveling but I like the looks of this. 👍
Port should be optional so it still shows up in the autogenerated help.
On Sat, Jan 20, 2018, 4:53 PM Codecov ***@***.***> wrote:
> Codecov
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448?src=pr&el=h1>
> Report
>
> Merging #1448
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448?src=pr&el=desc>
> into master
> <https://codecov.io/gh/node-serialport/node-serialport/commit/a819bcad53052b77596e77c9b7f849aa98c9944c?src=pr&el=desc>
> will *increase* coverage by 0.21%.
> The diff coverage is n/a.
>
> [image: Impacted file tree graph]
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448?src=pr&el=tree>
>
> @@ Coverage Diff @@## master #1448 +/- ##
> ==========================================+ Coverage 79.71% 79.93% +0.21%
> ==========================================
> Files 21 20 -1
> Lines 922 867 -55
> Branches 167 166 -1
> ==========================================- Hits 735 693 -42 + Misses 187 174 -13
>
> Impacted Files
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448?src=pr&el=tree> Coverage
> Δ
> lib/bindings/auto-detect.js
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448/diff?src=pr&el=tree#diff-bGliL2JpbmRpbmdzL2F1dG8tZGV0ZWN0Lmpz> 69.23%
> <0%> (-30.77%) ⬇️
> lib/bindings/darwin.js
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448/diff?src=pr&el=tree#diff-bGliL2JpbmRpbmdzL2Rhcndpbi5qcw==>
> ------------------------------
>
> Continue to review full report at Codecov
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448?src=pr&el=continue>
> .
>
> *Legend* - Click here to learn more
> <https://docs.codecov.io/docs/codecov-delta>
> Δ = absolute <relative> (impact), ø = not affected, ? = missing data
> Powered by Codecov
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448?src=pr&el=footer>.
> Last update a819bca...1e13043
> <https://codecov.io/gh/node-serialport/node-serialport/pull/1448?src=pr&el=lastupdated>.
> Read the comment docs
> <https://docs.codecov.io/docs/pull-request-comments>.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#1448 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AABlbvI2CFjEgYTaxJ0xDO1aQvskRgt_ks5tMqapgaJpZM4Rlq_G>
> .
>
|
|
I looked at listr on your recommendation and I think that prompt-list is a better fit for this use case. But maybe my cursory glance did not reveal something in listr that makes it a better fit. I think the error codes look good based on what you have said. What would you like me to do in order for this PR to be merged in? It looks good to me. Cheers! |
|
Just a bit about the error codes. I'm on vacation so I'll be mostly out of touch until next week. Looks good, I do approve 👍 |
|
Awesome, I've modified all of the error codes so that they are positive. Even the one that was I'll wait till you get back next week. Cheers, have a great break, and thank you for your time! 😄 |
Codecov Report
@@ Coverage Diff @@
## master #1448 +/- ##
==========================================
+ Coverage 79.71% 79.73% +0.02%
==========================================
Files 21 21
Lines 922 923 +1
Branches 167 169 +2
==========================================
+ Hits 735 736 +1
Misses 187 187
Continue to review full report at Codecov.
|
|
Hi @reconbot , I understand that you are on vacation so I don't want to ping too much, but by which date would you be available for us to pick this PR up and see if we can get it over the line? |
|
Thank you! Excited to see these changes go out in the next release! Your rock, keep up the great work! |
* Adding in port selection to serialport-term. This makes the terminal command much more useable when you need to open it multiple times in a row; especially when the names of the serialports are rapidly changing. * Fix the command line arguments. * Fixing eslint mistakes. * Make all of the negative errors positive in serialport-terminal.
* Adding in port selection to serialport-term. This makes the terminal command much more useable when you need to open it multiple times in a row; especially when the names of the serialports are rapidly changing. * Fix the command line arguments. * Fixing eslint mistakes. * Make all of the negative errors positive in serialport-terminal.
* Adding in port selection to serialport-term. This makes the terminal command much more useable when you need to open it multiple times in a row; especially when the names of the serialports are rapidly changing. * Fix the command line arguments. * Fixing eslint mistakes. * Make all of the negative errors positive in serialport-terminal.
This makes the terminal command much more usable when you need to open
it multiple times in a row; especially when the names of the serial ports
are rapidly changing.
At the moment, I'm doing a bunch of development on the nRF5 series of SOC's from Nordic Semiconductor and, because this is Bluetooth, I often have more than one SOC connected to my computer. Also, every time that I plug a new device in the Silicon Labs driver gives me a new virtual port for the device; making it a more lengthy than required process to reconnect to my logs. It would be better if it would just be:
At any rate, hopefully, you like these changes.