Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Node version output to t2 version command #679

Merged
merged 1 commit into from
Apr 16, 2016
Merged

Add Node version output to t2 version command #679

merged 1 commit into from
Apr 16, 2016

Conversation

wyze
Copy link
Member

@wyze wyze commented Apr 15, 2016

Fixes #667

This adds additional information to the t2 version command. If for some reason it fails to fetch the Node version, but was able to get the firmware version, it will output just that along with unknown for the Node version.

If it fails to get the firmware version first then it skips trying to get the Node version and just outputs unknown for both versions.

All the tests pass. There weren't any currently testing controller.tesselFirmwareVerion function, so I added those with this as well since I touched that function. I would love some feedback on the tests as I am pretty green with Sinon. :)

Also, note that Verion above should be Version, but I opted to not correct the spelling and would tackle that in a separate PR. Trying not to do so much here, but if you would like, I could rename that in this PR as well.

*/
Tessel.prototype.fetchCurrentNodeVersion = function() {
return this.simpleExec(['node', '--version'])
.then(function versionRead(version) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use arrow functions when you can.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I was following what I saw, and it was a lot of mixed between function keyword and arrow functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know.. 😄. Going forward we want to avoid adding more, and cleaning up as we go.

@wyze
Copy link
Member Author

wyze commented Apr 16, 2016

Updated based on @rwaldron feedback and rebased onto latest master.

})
.catch(() => {
logs.info(output('Firmware', firmwareVersion));
logs.info(output('Node', 'unknown'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rwaldron rwaldron merged commit 83c47d3 into tessel:master Apr 16, 2016
@rwaldron
Copy link
Contributor

Great work!!

@wyze wyze deleted the output-node-version branch April 16, 2016 18:27
@wyze
Copy link
Member Author

wyze commented Apr 16, 2016

Thanks! More to come.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants