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

Uses LAN and USB flags properly #360

Merged
merged 1 commit into from
Oct 2, 2015
Merged

Uses LAN and USB flags properly #360

merged 1 commit into from
Oct 2, 2015

Conversation

johnnyman727
Copy link
Contributor

Turns out this was the root cause of #358 and #238. Probably more issues too, they just weren't reported.

@@ -68,7 +72,7 @@ Tessel.list = function(opts) {
// Report that selected Tessel to the user
logs.info('Multiple Tessels found.');
if (tessel) {
logs.info('Will default to', tessel.name, '.');
logs.info('Will default to', tessel.name + '.');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was bugging me because it was print out as: Will default to Frank . with a space before the period.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now you use both , and + on the same line.

edit: nvm, I just realized that it's to skip the space between the name and the period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, perhaps sprintf is the reasonable way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should work and looks nice:

logs.info('Will default to %s.', tessel.name);

It uses util.format which is a builtin in Node that does this. That works with console.log as well 👍

> console.log('Hello %s.', 'world')
Hello world.

@johnnyman727
Copy link
Contributor Author

@LinusU since you already started, do you want to give it a full review?

@LinusU
Copy link
Contributor

LinusU commented Oct 2, 2015

Already on it ;)

@LinusU
Copy link
Contributor

LinusU commented Oct 2, 2015

Code looks good, tests pass locally and on CI.

The only thing is the one comment I made:

-              logs.info('Will default to', tessel.name + '.');
+              logs.info('Will default to %s.', tessel.name);

LGTM 👍

@johnnyman727
Copy link
Contributor Author

Awesome, thanks a million! Will make that change and merge.

johnnyman727 added a commit that referenced this pull request Oct 2, 2015
@johnnyman727 johnnyman727 merged commit 6308784 into master Oct 2, 2015
@johnnyman727 johnnyman727 deleted the jon-flag-clean-up branch October 2, 2015 05:42
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

2 participants