startersacademy / fullstack-project-01

Learning Management System
MIT License
0 stars 5 forks source link

Fix JSHint errors #55

Closed treyhunner closed 9 years ago

treyhunner commented 9 years ago

I would like everyone to review this pull request before it gets merged. Please make comments to discuss and add a :+1: if/when you understand and sign off on the changes made.

jeffreytu commented 9 years ago

:+1: Would be good to have this merged ASAP before we start merging in the week 2 pull requests.

goldlilys commented 9 years ago

Some files still failing in JS hint

treyhunner commented 9 years ago

@goldlilys I think I fixed it

goldlilys commented 9 years ago

The spacing indentation is different for sublime and webstorm when the content reaches the next line. First one is webstorm

      this.test.assertUrlMatch('/courses.html',
                               'currently in the courses page');
      this.echo('Page title is: ' + this.getTitle());
      test.assertTitle(this.getTitle(), 'about the courses title is good');
    });

VS 2nd is sublime text

      this.test.assertUrlMatch('/courses.html',
         'currently in the courses page');
      this.echo('Page title is: ' + this.getTitle());
      test.assertTitle(this.getTitle(), 'about the courses title is good');
  });

Which one do others prefer for consistency? If this matters at all. If not, this is good to go and merged.

michaelerobertsjr commented 9 years ago

:+1:

@treyhunner shouldn't editorconfig pick this line wrapping issue up if its properly configured?

treyhunner commented 9 years ago

@michaelerobertsjr there are no EditorConfig options for line wrapping. This is an issue of indentation vs. alignment.

@goldlilys said Sublime Text re-indents with just 2 spaces instead of aligning hanging wrapped lines. What does WebStorm do?

jeffreytu commented 9 years ago

@goldlilys Visually, I would prefer the first. I'm using Sublime, so I have the same issue of it just indenting 2 spaces.

jadedsurfer commented 9 years ago

The maxlen linting rule says no line should be over 80 columns so you should have no soft wraps that would be controlled by the editor (leading to the differences you mention above). The team should decide what kind of alignment they want for lines that are too long. Here's what the google js styleguide has to say about it: "When possible, all function arguments should be listed on the same line. If doing so would exceed the 80-column limit, the arguments must be line-wrapped in a readable way. To save space, you may wrap as close to 80 as possible, or put each argument on its own line to enhance readability. The indentation may be either four spaces, or aligned to the parenthesis." So in the above example, both would be stylistically correct according to google.

jadedsurfer commented 9 years ago

+1 for the merge of this branch.

goldlilys commented 9 years ago

:thumbsup: cool, good to go for merging @treyhunner