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

Fix some style choices causing JSHint warnings #234

Merged
merged 1 commit into from
Jul 26, 2014
Merged

Fix some style choices causing JSHint warnings #234

merged 1 commit into from
Jul 26, 2014

Conversation

thethomaseffect
Copy link
Collaborator

This pull request should contain zero functional changes. If there are any then please comment and I'll amend it before merging. The main changes are as follows:

  • For multiline var assignments and ternary expressions, place operators before new line instead of after. This is the style JSHint advocates and much more common among the JS community.
  • Change some statements to multiline to make them more readable (311-314, 446-448).
  • Added some explicit newline characters on help information instead of joining empty strings.

@rlidwka
Copy link
Contributor

rlidwka commented Jul 26, 2014

For multiline var assignments and ternary expressions, place operators before new line instead of after.

Bad idea. In case of var assignments, trailing comma is easy to miss, therefore prone to errors.

Do you see an error here?:

var self = this,
   option = new Option(flags, description)
   oname = option.name(),
   name = camelcase(oname);

And how about this one?:

var self = this
  , option = new Option(flags, description)
    oname = option.name()
  , name = camelcase(oname);

Multiline statements with leading operators don't have that kind of error, but leading operators are much more readable than trailing ones.

I could easily read this, because indentation + leading ternary shows code structure nicely:

    return cmd._name
       + (cmd._alias
         ? '|' + cmd._alias
         : '')
       + (cmd.options.length
         ? ' [options]'
         : '') + ' ' + args
       + (cmd.description()
         ? '\n   ' + cmd.description()
         : '')
       + '\n';
   }).join('\n').replace(/^/gm, '    ')

But this looks like a random mess of strings with a different indentation. I noticed ternary operators only after a while:

     return cmd._name + (cmd._alias ?
         '|' + cmd._alias :
         '') +
       (cmd.options.length ?
         ' [options]' :
         '') +
       ' ' +
       args + (cmd.description() ?
         '\n   ' + cmd.description() :
         '') +
       '\n';
   }).join('\n').replace(/^/gm, '    '),

Added some explicit newline characters on help information instead of joining empty strings

Joining empty strings is actually an attempt to make code look like the output. If you're using newline characters, spacing between lines in the code and in the output is different. So "-1" on that as well.

I could agree that i > 0 && (lastOpt = this.optionFor(args[i-1])); looks weird, but generally I'd just leave current code as is.

I'd say don't use JSHint, it has some less than sane defaults, and it can't really be configured. Instead, you can use ESLint, which you can configure as you like. And don't blindly follow linter tools (before you review each and every one config option) - it is almost always a bad idea.

@SomeKittens
Copy link
Collaborator

I'm with @rlidwka on comma-first.

@thethomaseffect
Copy link
Collaborator Author

Cool, I'll drop the changes to comma first code and the help text

@thethomaseffect
Copy link
Collaborator Author

Removed old commit and made a new one so history is cleaner. Let me know if there's still any issues.

@rlidwka
Copy link
Contributor

rlidwka commented Jul 26, 2014

Looks good to me.

thethomaseffect added a commit that referenced this pull request Jul 26, 2014
Changed some loc to make them more readable
@thethomaseffect thethomaseffect merged commit 4e839c2 into tj:master Jul 26, 2014
@thethomaseffect
Copy link
Collaborator Author

Great, thanks for the review!

@SomeKittens
Copy link
Collaborator

+1 for clean commit logs. Thanks for standardizing the code a bit!

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.

3 participants