Skip to content

Some JSHint fixes#458

Merged
Martii merged 6 commits into
OpenUserJS:masterfrom
jerone:jshint
Dec 2, 2014
Merged

Some JSHint fixes#458
Martii merged 6 commits into
OpenUserJS:masterfrom
jerone:jshint

Conversation

@jerone
Copy link
Copy Markdown
Contributor

@jerone jerone commented Nov 28, 2014

Handled code issues:

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.

@jerone 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
Comment thread libs/remove.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This probably fixes a bug. Good find.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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
Copy link
Copy Markdown
Member

Martii commented Dec 1, 2014

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
Copy link
Copy Markdown
Member

Martii commented Dec 1, 2014

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

@jerone
Copy link
Copy Markdown
Contributor Author

jerone commented Dec 1, 2014

@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
Copy link
Copy Markdown
Contributor Author

jerone commented Dec 1, 2014

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:

@Martii
Copy link
Copy Markdown
Member

Martii commented Dec 1, 2014

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

…into jshint

Conflicts:
	controllers/admin.js
	controllers/user.js
@Martii
Copy link
Copy Markdown
Member

Martii commented Dec 1, 2014

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
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member

Martii commented Dec 1, 2014

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 Martii removed the PR READY This is used to indicate that a pull request (PR) is ready for evaluation. label Dec 1, 2014
Comment thread controllers/auth.js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 here

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug You've guessed it... this means a bug is reported. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment.

Development

Successfully merging this pull request may close these issues.

4 participants