Fix jshint warnings #11109

5earle · 2018-02-06T03:47:45Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #9153

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

5earle · 2018-02-06T03:49:31Z

Addresses #9153

Remove JSHint warnings

Splaktar

Let's just disable these two rules via the following in .jshintrc:

  "sub": true,
  "multistr": true,

Splaktar

Can you please revert the changes to the multiline strings and references like clickedItem['name']? Then please squash your commits.

5earle · 2018-02-11T07:10:49Z

ok so just make changes to the .jshint and the missing semicolons?

Splaktar · 2018-02-11T08:33:29Z

Yes please, also the Confusing use of '!' should be fixed (not excluded).

Splaktar

This is coming along well. Almost ready 😄. I just had a couple more minor comments.

Splaktar · 2018-02-11T21:47:48Z

.jshintrc

@@ -1,4 +1,7 @@
 {
+  "sub": true,
+  "multistr": true,
+  "-W018": true,


Sorry, I was trying to say that we should fix the issues where "confusing use of '!'" gave a warning, not exclude the rule.

Ok i will look into it, but I'm not sure why its showing a warning

Splaktar · 2018-02-11T21:50:06Z

src/components/autocomplete/demoBasicUsage/script.js

+               Nebraska, Nevada, New Hampshire, New Jersey, New Mexico, New York, North Carolina,\
+               North Dakota, Ohio, Oklahoma, Oregon, Pennsylvania, Rhode Island, South Carolina,\
+               South Dakota, Tennessee, Texas, Utah, Vermont, Virginia, Washington, West Virginia,\
+               Wisconsin, Wyoming';


Can you please remove these added spaces? It should be indented 8 spaces, not 9.

5earle · 2018-02-13T17:44:48Z

these are giving me some problems to fix

if (!!value === targetValue)
if (setVisible.queued && setVisible.value === !!value

Splaktar · 2018-02-13T19:16:04Z

OK, yeah it's not trivial to fix those. Let's go with your first instinct and just exclude that rule in the .jshintrc file for now. Thanks for looking into it!

Splaktar

Code looks good now, thank you. Please squash your commits and then we can get this submitted to be merged.

Splaktar · 2018-03-13T05:29:24Z

I would like to get this merged into 1.1.8 as we're hoping to release it this week, can you please squash your commits so that we can merge this?

add sub:true and multistr=true to .jshintrc revert and remove confusing use of ! warning update fix fix space exclude warnings

5earle · 2018-03-13T14:24:06Z

ok so i was trying to do the rebase.

after squash attempt:

commit 8752694
Author: ew [email protected]
Date: Mon Feb 5 22:43:56 2018 -0500
Fix jshint warnings
add sub:true and multistr=true to .jshintrc
revert and remove confusing use of ! warning
update fix
fix space
exclude warnings

Fail to Push:

! [rejected] fix-jshint -> fix-jshint (non-fast-forward)
error: failed to push some refs to 'https://github.com/5earle/material.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Splaktar · 2018-03-13T19:31:10Z

Have you tried force push? That's needed when re-writing a remote branch's history like we want to do when squashing commits.

5earle · 2018-03-13T23:14:45Z

ok that did the trick

add sub:true and multistr=true to .jshintrc revert and remove confusing use of ! warning update fix fix space exclude warnings

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Feb 6, 2018

Splaktar self-assigned this Feb 11, 2018

Splaktar added P5: nice to have These issues will not be fixed without community contributions. in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Feb 11, 2018

Splaktar added this to the 1.1.8 milestone Feb 11, 2018

Splaktar self-requested a review February 11, 2018 02:47

Splaktar suggested changes Feb 11, 2018

View reviewed changes

Splaktar reviewed Feb 11, 2018

View reviewed changes

Splaktar added the needs: squash commits label Feb 18, 2018

Splaktar approved these changes Feb 18, 2018

View reviewed changes

Fix jshint warnings

8752694

add sub:true and multistr=true to .jshintrc revert and remove confusing use of ! warning update fix fix space exclude warnings

5earle force-pushed the fix-jshint branch from 10947d0 to 8752694 Compare March 13, 2018 23:13

Splaktar added pr: merge ready This PR is ready for a caretaker to review type: chore and removed needs: squash commits in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Mar 14, 2018

mmalerba merged commit cec409a into angular:master Mar 15, 2018

5earle deleted the fix-jshint branch March 16, 2018 00:08

chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018

Fix jshint warnings (angular#11109)

de3e505

add sub:true and multistr=true to .jshintrc revert and remove confusing use of ! warning update fix fix space exclude warnings

Splaktar pushed a commit that referenced this pull request Jul 31, 2018

Fix jshint warnings (#11109)

dee7737

add sub:true and multistr=true to .jshintrc revert and remove confusing use of ! warning update fix fix space exclude warnings

Fix jshint warnings #11109

Fix jshint warnings #11109

Uh oh!

Conversation

5earle commented Feb 6, 2018 • edited by Splaktar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

5earle commented Feb 6, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

5earle commented Feb 11, 2018

Uh oh!

Splaktar commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

5earle Feb 12, 2018

Choose a reason for hiding this comment

Uh oh!

Splaktar Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

5earle commented Feb 13, 2018

Uh oh!

Splaktar commented Feb 13, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Mar 13, 2018

Uh oh!

5earle commented Mar 13, 2018

Uh oh!

Splaktar commented Mar 13, 2018

Uh oh!

5earle commented Mar 13, 2018

Uh oh!

Uh oh!

5earle commented Feb 6, 2018 •

edited by Splaktar

Loading

Splaktar commented Feb 11, 2018 •

edited

Loading