Some JSHint fixes #458

jerone · 2014-11-28T14:13:28Z

Handled code issues:

Converted ~~all~~ == and != to === and !==.
Removed unused variables (excluding parameters and isPro, isDev & isDbg).
Removed unnecessary semicolons.
Added missing semicolons.
Fixed some issues where wrong variable names were used.
- Issue 264prefix a parm lists #267 at 82dc007#diff-590bb4d75f87edbecb61fa8487493291L123
Renamed already defined variable/function.

Not handled:

Unused parameter variables.
Lots of unused variables isPro, isDev & isDbg. +/-90% of the files have one these unused variables…
User.edit still contains a lot of bugs, but this function is never used.
Empty code blocks.
Indentation
Whitespace

No functionality should have been changed!

Node modules are ignored.

Ref #406

A change to get the code to a higher level of quality and consistency.

Zren · 2014-11-28T19:25:10Z

libs/remove.js

@@ -126,7 +126,7 @@ exports.findDeadorAlive = function (aModel, aQuery, aUser, aCallback) {
        return aCallback(false, null, aRemoved);
      }

-      aCallback(false, new model(aRemoved.content), aRemoved); // TODO: Ambiguous
+      aCallback(false, new aModel(aRemoved.content), aRemoved); // TODO: Ambiguous


This probably fixes a bug. Good find.

@jerone since you have appeared to resolve the ambiguity please remove the line note too associated with this fix. Won't be available until tomorrow eve for a little bit but looks good so far. Thanks.

Martii · 2014-12-01T07:56:07Z

hmmm direct pr checkout out of this doesn't let me edit my profile at http://localhost:8080/users/Marti e.g. it doesn't think that it's me.

Martii · 2014-12-01T08:01:36Z

Remaining sequential local merge of current master (50589f1), 451, 455 then 458 causes merge conflict.

jerone · 2014-12-01T15:57:45Z

@Martii commented on 1 dec. 2014 08:56 CET:

hmmm direct pr checkout out of this doesn't let me edit my profile at http://localhost:8080/users/Marti e.g. it doesn't think that it's me.

Will check tonight. Thnx for testing.

jerone · 2014-12-01T19:15:57Z

Ok, found the problem: https://github.com/OpenUserJs/OpenUserJS.org/pull/458/files#diff-664f851728e6fc6ec664075df484505cR246
authedUser._id is a string, while user._id is actually an object with among others a method toString(). Also user has the property user.id available, while authedUser doesn't. Both are from the model User.
The problem I think is that user is serialized, because it comes from the session.

This problem is a little bit bigger then I hoped it was and requires some discussion (not here) about a real solution. Because of that I'll revert all authedUser._id === user._id back changes to authedUser._id == user._id.

Some reading:

…into jshint

Martii · 2014-12-01T21:44:30Z

Needs master merge as I said above at #458 (comment) and complete retesting here.

…into jshint Conflicts: controllers/admin.js controllers/user.js

Martii · 2014-12-01T23:09:27Z

I appear not to "own" my script at http://localhost:8080/scripts/Marti/uso_-_Count_Issues/source e.g. it says "Submit Code as Fork" instead of "Submit Code".

sizzlemctwizzle · 2014-12-01T23:18:56Z

Leave all equality checks alone. When == or != are used, it was done intentionally. Defer to the programmer's judgement for this instead of blindly following convention and risk breaking things.

Martii · 2014-12-01T23:22:25Z

I kind of figured that... but I thought I'd give JSHint one last try... I'd still rather see these things split up into smaller chunks... e.g. we do need the semicolon fixes and probably a few other misc TODO fixes... but in separate PRs... I'd do cherry picking distinct commits but that is on hold since it allows chaining changes which sometimes those might need to be discussed first.

Anywho my fakes3 crashed in another console tab which is why I was having issues a few moments ago... reverted that system level patch and it's okay now.

Martii · 2014-12-01T23:23:39Z

controllers/auth.js

@@ -122,7 +121,7 @@ exports.callback = function (aReq, aRes, aNext) {
  if (openIdStrategies[strategy]) {
    strategyInstance._verify = function (aId, aDone) {
      verifyPassport(aId, strategy, username, aReq.session.user, aDone);
-    }
+    };


Martii · 2014-12-01T23:26:40Z

libs/remove.js

@@ -50,7 +50,7 @@ function removeable(aModel, aContent, aUser, aCallback) {
 exports.removeable = removeable;

 function remove(aModel, aContent, aUser, aReason, aCallback) {
-  var remove = new Remove({
+  var removeModel = new Remove({


-0+ here... JSHint specific I think.

Martii · 2014-12-02T19:39:10Z

I'm -1 on using == in favor of ===. It's always possible to convert to the same datatype.

@jerone
So after you update this again you'll need to convert and retest all of them then. You still need to test all the logic paths but so will I (even though you are just about stopping my potential prs due to retesting). e.g. because this encompasses more than one thing ... it's still quite possible this will be closed and possibly cherry picked for items that need to be done versus what JSHint thinks should be done. Because of our STYLEGUIDE I'm not exactly thrilled with the derivation in some of the == vs === myself... I prefer not using === in my own code because it is lengthy and detracts from JavaScript's uniqueness as a whole... but that's my opinion. Some of these fixes are from older code when things weren't made clear to everyone contributing... so the ball is in your court with this PR... What do you want to do?

jerone · 2014-12-02T20:13:33Z

I reverted all === comparison that touch objects coming from the database because of #458 (comment)

Martii · 2014-12-02T20:16:31Z

controllers/scriptStorage.js

@@ -234,7 +233,6 @@ exports.getMeta = function (aChunks, aCallback) {
  // get the user script header.
  var str = '';
  var i = 0;
-  var len = aChunks.length;
  var header = null;

  for (; i < aChunks.length; ++i) {


Just as a FYI I see an incomplete optimization from someone here which is why JSHint plucked line 237 out.

Martii · 2014-12-02T21:08:25Z

Fully retested on dev usage at this point... just need only a PR READY when you are and I'll give it a +1

Some JSHint fixes +1 merge

Some JSHint fixes;

ba7c0c0

jerone added PR READY This is used to indicate that a pull request (PR) is ready for evaluation. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. labels Nov 28, 2014

Zren reviewed Nov 28, 2014
View reviewed changes

Removed Ambiguous comment;

dfb3dfc

jerone added 2 commits December 1, 2014 20:19

Revert all user._id === user._id changes;

207e147

Merge branch 'master' of https://github.com/OpenUserJs/OpenUserJS.org …

2e9779d

…into jshint

Merge branch 'master' of https://github.com/OpenUserJs/OpenUserJS.org …

a9668ad

…into jshint Conflicts: controllers/admin.js controllers/user.js

Martii removed the PR READY This is used to indicate that a pull request (PR) is ready for evaluation. label Dec 1, 2014

Martii reviewed Dec 1, 2014
View reviewed changes

Martii mentioned this pull request Dec 2, 2014

_template.js usage #464

Closed

Revert all === comparison with db models;

59a68a0

Martii reviewed Dec 2, 2014
View reviewed changes

jerone added the PR READY This is used to indicate that a pull request (PR) is ready for evaluation. label Dec 2, 2014

Martii added a commit that referenced this pull request Dec 2, 2014

Merge pull request #458 from jerone/jshint

8fdba9a

Some JSHint fixes +1 merge

Martii merged commit 8fdba9a into OpenUserJS:master Dec 2, 2014

Martii added bug You've guessed it... this means a bug is reported. and removed PR READY This is used to indicate that a pull request (PR) is ready for evaluation. labels Dec 2, 2014

jerone deleted the jshint branch December 2, 2014 21:33

github-actions bot locked as resolved and limited conversation to collaborators May 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some JSHint fixes #458

Some JSHint fixes #458

jerone commented Nov 28, 2014

Zren Nov 28, 2014

Martii Nov 29, 2014

Martii commented Dec 1, 2014

Martii commented Dec 1, 2014

jerone commented Dec 1, 2014

jerone commented Dec 1, 2014

Martii commented Dec 1, 2014

Martii commented Dec 1, 2014

sizzlemctwizzle commented Dec 1, 2014

Martii commented Dec 1, 2014

Martii Dec 1, 2014

Martii Dec 1, 2014

Martii commented Dec 2, 2014

jerone commented Dec 2, 2014

Martii Dec 2, 2014

Martii commented Dec 2, 2014

Some JSHint fixes #458

Some JSHint fixes #458

Conversation

jerone commented Nov 28, 2014

Zren Nov 28, 2014

Choose a reason for hiding this comment

Martii Nov 29, 2014

Choose a reason for hiding this comment

Martii commented Dec 1, 2014

Martii commented Dec 1, 2014

jerone commented Dec 1, 2014

jerone commented Dec 1, 2014

Martii commented Dec 1, 2014

Martii commented Dec 1, 2014

sizzlemctwizzle commented Dec 1, 2014

Martii commented Dec 1, 2014

Martii Dec 1, 2014

Choose a reason for hiding this comment

Martii Dec 1, 2014

Choose a reason for hiding this comment

Martii commented Dec 2, 2014

jerone commented Dec 2, 2014

Martii Dec 2, 2014

Choose a reason for hiding this comment

Martii commented Dec 2, 2014