Currently working on this in wip/jshint

Anything new on this?

Well, the jshint-file is there (.jshintrc in the root) but I don't think anyone is using it (including me). 
Maybe we should add it to the test suite and ask people to run the suite before putting patches on bz. Unfortunately we don't have fancy integration of the test suite with the patch review so we'll have to remember to ask if the patches has been run. 

Keep this open and I'll cook up a patch soonish to fix the errors from jshint and then we'll start using it for real.

Created attachment 288625 [details] [review]
Cleanup: Remove unused imports

We import a bunch of modules we don't use in a lot of files. Remove all
such imports.

Created attachment 288626 [details] [review]
JSHint: Allow multi line strings

JSHint defaults to not allow multi line strings, probably because of
buggy browser implementations.

This shouldn't affect us, and is handy when doing dbus calls so allow
this.

Created attachment 288627 [details] [review]
Cleanup: Remove type coercion compares

The '==' and '!=' operators in JS is a bit weird since it tries to do
automatic and sometimes unexpected type coercions instead of a strict
comparison.

In Maps we have decided to use the '===' and '!==' operators instead
since they do stricter checks.

This patch converts two cases where '!=' passed through review.

Created attachment 288628 [details] [review]
Cleanup: Add some missing semicolons

Semi-colons aren't strictly necessary in JavaScript, but not using them
can sometimes bite you, so we have decided to use semi-colons throughout
our code base.

Fix three cases where semi-colons were missing.

Created attachment 288629 [details] [review]
JSHint: Allow late function definitions

By default JSHint warns when you use functions before they are defined.
That's not really necessary, and it's sometimes convenient to define
functions in "backwards" order.

This patch makes JSHint only complain about using variables before they
are declared, not functions.

Created attachment 288630 [details] [review]
Sidebar: Reorder definitions

Define InstructionRow before Sidebar instead of after it. Fixing a
JSHint error about usages before definitions.

Created attachment 288631 [details] [review]
JSHint: Allow unsafe lineendings

This is meant as a way to protect oneself from beeing bitten by
JavaScripts Automatic Semicolon Insertion.

Since this is already covered by the "asi" option in JSHint and it
warns on existing code with operators first we'll relax this check.

Created attachment 288632 [details] [review]
HTTP: Supress loopfunc warning for now

JSHint warns when you create functions inside loops since closures in
combination with function scoping can be very unintuitive.

We should probably keep that behaviour but that means we get a warning
in the http module (for entirely safe code), so supress that warning.

These patches should fix all warnings from JSHint.

Regarding how and when to run this, I think the simplest is just to document somewhere on how to install jshint, since then it's just a matter of running:
  
  $ jshint src/*.js

from the root folder of gnome-maps.

A quick guide on how to install jshint (so that whoever reviews this can test):

Fedora specific:

  $ pkcon install npm

Ubuntu specific:
  
  $ sudo apt-get install npm nodejs-legacy

Add ~/.local/bin/ to the path if it isn't already

  $ echo "export PATH=$HOME/.local/bin:\$PATH" >> ~/.bashrc

Set up node.js to use ~/.local/:

  $ echo "prefix = ${HOME}/.local" >> ~/.npmrc
  $ echo "export NODE_PATH=$HOME/.local/lib/node_modules/:\$NODE_PATH" >> ~/.bashrc
  $ source ~/.bashrc

Install jshint

  $ npm install -g jshint

Review of attachment 288625 [details] [review]:

That's a lot of imports, Batman!

Review of attachment 288626 [details] [review]:

Ack

Review of attachment 288627 [details] [review]:

I am surprised we didn't have more of these, actually. Way to go us!

For reference: http://www.sitepoint.com/javascript-truthy-falsy/

Review of attachment 288628 [details] [review]:

Okay!

Review of attachment 288629 [details] [review]:

Sure.

In the future, feel free to commit stuff like this (tuning of .jsinitrc) without going through bz.

Review of attachment 288629 [details] [review]:

Noop

Review of attachment 288630 [details] [review]:

Hmm, is this the only case of this in the code base?

Review of attachment 288631 [details] [review]:

Ok!

Review of attachment 288632 [details] [review]:

Not sure about this. Are we likely to get more of comments like this? I don't want to see them all over the place.
Is this a super special case?

Review of attachment 288630 [details] [review]:

According to jshint: yes.

We access functions before they are defined a lot but that's fine so long as they are defined like:
   function f() { }
instead of
   let f = function() {}
because the JS implementation will find them before running any other code in the file I believe.

Review of attachment 288632 [details] [review]:

I understand that, and felt a bit unsure about this. 

Thing is I plan on making a patch converting all forEach to for..of-loops after this[1] so didn't want to spend time to refactor this.

1: gnome-shell has been using for..of-loops for a while now so I think it's safe for us to move over as well.

Attachment 288625 [details] pushed as 18aca41 - Cleanup: Remove unused imports
Attachment 288626 [details] pushed as 57369a3 - JSHint: Allow multi line strings
Attachment 288627 [details] pushed as f343b9b - Cleanup: Remove type coercion compares
Attachment 288628 [details] pushed as e0daa16 - Cleanup: Add some missing semicolons
Attachment 288629 [details] pushed as c96417c - JSHint: Allow late function definitions
Attachment 288631 [details] pushed as be4e84d - JSHint: Allow unsafe lineendings

Added the guide above to the wiki[1].

Would be nice to get the other changes pushed so that JSHint produces 0 warnings.

1: https://wiki.gnome.org/Apps/Maps/JSHint

Review of attachment 288630 [details] [review]:

Allright.

Review of attachment 288632 [details] [review]:

So this will not be needed after converting to for...of-loops? Then do we really need to add this now?

Review of attachment 288632 [details] [review]:

No, we can definitely live with one warning for now I think. So let's skip this one, push the other and close this. :)

Attachment 288630 [details] pushed as 71f8ea6 - Sidebar: Reorder definitions