JSHint fixes (including indentation & whitespace); #406

jerone · 2014-11-06T19:43:32Z

This PR includes:

JSHint warning fixes.
Style consistency (including indentation & whitespace).

No functionality should have been changed!

Node modules & ace has been ignored.

Some files still have warnings, but those warnings needs addressing later as it might involve functionality changes:

controllers/_template.js
controllers/scriptStorage.js
controllers/user.js
libs/modelQuery.js
libs/modifySessions.js
libs/muExpress.js
libs/remove.js
libs/templateHelpers.js

Hoping that this can reviewed asap to keep conflicts as a minimum.

Martii · 2014-11-06T20:19:06Z

Some of these fixes look like they are needed for consistency... however this is section blocked by #262 since I'm sure it deals with vote/flag/remove....

Placing on hold for the time being with a -1 until @sizzlemctwizzle can pipe in... there is still a lot of code normalizing to do, as well with strict mode too, but I've had to wait on that.

jerone · 2014-11-06T20:30:46Z

@Martii commented on 6 nov. 2014 21:19 CET:

Some of these fixes look like they are needed for consistency... however this is section blocked by #262 since I'm sure it deals with vote/flag/remove....

Placing on hold for the time being with a -1 until @sizzlemctwizzle can pipe in... there is still a lot of code normalizing to do, as well with strict mode too, but I've had to wait on that.

I understand your reasoning, where can I follow the current code of that issue?

Martii · 2014-11-06T20:33:35Z

where can I follow the current code of that issue?

#262

I can re-ask him in private if he doesn't mind being over trodden in that area but no promises.

jerone · 2014-11-06T20:38:45Z

@Martii commented on 6 nov. 2014 21:33 CET:

#262

I can re-ask him in private if he doesn't mind being over trodden in that area but no promises.

Should have pinged him ( @sizzlemctwizzle ) myself, this private contacting is not a good way to work together and keep discussions public.

Martii · 2014-11-06T21:00:28Z

He is in the public and so am I ... he just doesn't have a status update yet... sometimes everyone can use a reminder to go view a specific area... so non-issue there.

Zren · 2014-11-06T22:07:24Z

I'm fairly opposed on these big formatting commits as they break code I'm too lazy to submit (not a good reason) and introduce bugs as there are no tests (not that I see any in this case).

One rule I dislike about this commit has to be:

if () return; and if ()\n return; are fine and don't need to be wrapped in {}. The \n is optional but is good to have. My rules are: {} is not optional if the statement uses more than a single line. {} is not optional if there is an else statement attached.

But now that I look at our styleguide, which I've totally read before, apparently the consistent {} is a thing. :/

Thanks for catching all the missing/extra ; though.

I'll grumble and sigh when this is merged. Just be sure it's a quick clean cut.

jerone · 2014-11-06T22:15:03Z

My personal opinion is to get this in as soon as possible; keep the base as consistent as possible and follow the styleguide as best possible, from the beginning.

There we're no open PR's, so it looked the best time.

Martii · 2014-11-06T22:18:59Z

I would like us to slowly actually follow ./controllers/_template.js a little bit more... I know there are situations where we can't but overall it is my understanding that file is the preferred controller format.

And it's still "up in the air" about one liners although they should always have braces from my understanding of the STYLEGUIDE.md. JSHint isn't always correct either as proven multiple times in the past... but there are a few things that could be isolated from this PR, slowly, and merged in.

Zren · 2014-11-06T22:22:22Z

libs/passportVerify.js

@@ -23,50 +23,49 @@ exports.verify = function (aId, aStrategy, aUsername, aLoggedIn, aDone) {

  findDeadorAlive(User, { 'auths': digest }, true,
    function (aAlive, aUser, aRemoved) {
-      var pos = aUser ? aUser.auths.indexOf(digest) : -1;
-      if (aRemoved) { aDone(null, false, 'user was removed'); }
+    var pos = aUser ? aUser.auths.indexOf(digest) : -1;


Why is the function body indented at the same level as the definition? Tabs vs Spaces issue?

Because it wants function to be after the previous comma ,.

Zren · 2014-11-06T22:26:34Z

There we're no open PR's, so it looked the best time.

It's all good. Make sure to fix whatever the merge conflict your other patches made.

…into jshint Conflicts: controllers/issue.js

jerone · 2014-11-06T22:50:55Z

@Zren commented on 6 nov. 2014 23:26 CET:

Make sure to fix whatever the merge conflict your other patches made.

Done.

Martii · 2014-11-06T23:04:59Z

There's definitely some issues with JSHint here... I'm against this merge but I will wait a bit to see if sizzle is available... JSHint should NEVER change TODO comment lines as it moves where a change should be done and there is also some compliance with our STYLEGUIDE that JSHint doesn't pay attention to... should know more later.

jerone · 2014-11-06T23:07:44Z

JSHint should NEVER change TODO comment lines

That's not an JSHint issue. Can revert that change. Please point to the corresponding lines.

Martii · 2014-11-06T23:14:42Z

Please point to the corresponding lines.

As I stated above this is on HOLD regardless of styling because it's section blocked... I will get back to it at a later point... otherwise if you are impatient I'll just close it as invalid.

Martii · 2014-11-07T05:13:19Z

app.js

-  secret: sessionSecret,
-  store: sessionStore
-}));
+    secret: sessionSecret,


moot whitespace adjustment

Martii · 2014-11-07T05:25:22Z

controllers/script.js

-      .caseInsensitive(script.issuesCategorySlug), open: {$ne: false} });
+  var scriptOpenIssueCountQuery = Discussion.find({
+    category: scriptStorage
+      .caseInsensitive(script.issuesCategorySlug), open: { $ne: false }


Period can be considered to be an operator so this is incorrect.

Martii · 2014-11-07T07:51:33Z

.jshintignore

@@ -0,0 +1 @@
+**/ace/**.js


Not everyone uses JSHint... and I think Zren mentioned to you that this could be done in your .git folder.

Follow-up #97

#97 is currently stale (not to mention problematic) and you might notice I didn't include it in the #249 milestones. See #97 (comment).

Martii · 2014-11-07T18:25:11Z

Probably should be discussed in the PR comments (not inline).

If this were merged, which I'm -1 on it still, the inline comments become part of this PR historically... however if you want it here I'll echo it:

I would feel more comfortable with separate PRs/commits to cherry pick... one for the semi-colons, one for this, one for that... one big huge, inclusive, chunk is bound to increase the problems in our code.

e.g. too many changes in this PR that are controversial at this point in time... there is the needed semi-colon fixes, function space indentions, etc (e.g. "nice catches") but each would be best served as a standalone PR.

jerone · 2014-11-08T15:11:45Z

So how do we proceed? because I feel a lot of resistance to this PR.
Do I send a PR for each file or each type of fix separately or both...?

Martii · 2014-11-08T18:17:29Z

So how do we proceed?

...

I would feel more comfortable with separate PRs/commits to cherry pick

...

there is the needed semi-colon fixes, function space indentions, etc (e.g. "nice catches") but each would be best served as a standalone PR.

In general I'm not thrilled with that auto-indent thing you are using and would suggest that you omit that. You can either submit a commit to restore all the whitespace/linebreaks back to where it was in this PR or close it and submit new ones... left it open for you to decide... although since you are making files I'm not quite sure how that would work for you removing the dot files... so probably best to do another PR unless you or GH have a trick up your sleeve.

Martii · 2014-11-08T19:40:30Z

controllers/user.js

@@ -1496,7 +1500,8 @@ exports.flag = function (aReq, aRes, aNext) {
    var fn = flagLib[unflag && unflag === 'unflag' ? 'unflag' : 'flag'];
    if (aErr || !aUser) { return aNext(); }

-    fn(User, aUser, aReq.session.user, function (aFlagged) { // TODO: Non-descript function name
+    fn(User, aUser, aReq.session.user, function (aFlagged) {
+      // TODO: Non-descript function name


This is a prime example of why they shouldn't be moved... the TODO is actually meant for the statement before it.

jerone · 2014-11-08T19:45:05Z

@Martii commented on 8 nov. 2014 19:17 CET:

So how do we proceed?

...

I would feel more comfortable with separate PRs/commits to cherry pick

...

@jerone commented on 8 nov. 2014 16:11 CET:

Do I send a PR for each file or each type of fix separately or both...?

Closing this PR as I'm probably going to send separate PR's.

Martii · 2014-11-08T19:45:27Z

libs/remove.js

@@ -44,7 +44,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({


here we go again... newly renamed identifier.

Martii · 2014-11-18T05:07:54Z

libs/modelQuery.js

@@ -136,7 +138,7 @@ var applyModelListQueryFlaggedFilter = function (aModelListQuery, aOptions, aFla
      if (aFlaggedQuery == 'true') {


btw seems to have missed this one

jerone added 2 commits November 6, 2014 20:28

JSHint fixes (including indentation & whitespace);

0a04f0a

JSHint fixes (including indentation & whitespace);

0adffb6

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

Zren reviewed Nov 6, 2014
View reviewed changes

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

da64506

…into jshint Conflicts: controllers/issue.js

Martii reviewed Nov 7, 2014
View reviewed changes

app.js

secret: sessionSecret,

store: sessionStore

}));

secret: sessionSecret,

Copy link

Member

Martii Nov 7, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moot whitespace adjustment

Martii reviewed Nov 7, 2014
View reviewed changes

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

Martii reviewed Nov 7, 2014
View reviewed changes

Martii reviewed Nov 8, 2014
View reviewed changes

jerone closed this Nov 8, 2014

Martii reviewed Nov 8, 2014
View reviewed changes

jerone deleted the jshint branch November 8, 2014 19:47

Martii reviewed Nov 18, 2014
View reviewed changes

Martii mentioned this pull request Nov 25, 2014

introduce grunt #97

Closed

jerone mentioned this pull request Nov 28, 2014

Some JSHint fixes #458

Merged

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

JSHint fixes (including indentation & whitespace); #406

JSHint fixes (including indentation & whitespace); #406

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

Zren commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

Zren Nov 6, 2014

jerone Nov 7, 2014

Zren commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

Martii Nov 7, 2014

Martii Nov 7, 2014

Martii Nov 7, 2014

jerone Nov 7, 2014

Martii Nov 7, 2014

Martii commented Nov 7, 2014

jerone commented Nov 8, 2014

Martii commented Nov 8, 2014

Martii Nov 8, 2014

jerone commented Nov 8, 2014

Martii Nov 8, 2014

Martii Nov 18, 2014

		@@ -136,7 +138,7 @@ var applyModelListQueryFlaggedFilter = function (aModelListQuery, aOptions, aFla
		if (aFlaggedQuery == 'true') {

		@@ -0,0 +1 @@
		/ace/.js

JSHint fixes (including indentation & whitespace); #406

JSHint fixes (including indentation & whitespace); #406

Conversation

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

Zren commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zren commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

jerone commented Nov 6, 2014

Martii commented Nov 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martii commented Nov 7, 2014

jerone commented Nov 8, 2014

Martii commented Nov 8, 2014

Choose a reason for hiding this comment

jerone commented Nov 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment