Some misc STYLEGUIDE.md conformance in .../scriptStorage.js#713
Merged
Martii merged 1 commit intoOpenUserJS:masterfrom Aug 18, 2015
Merged
Some misc STYLEGUIDE.md conformance in .../scriptStorage.js#713Martii merged 1 commit intoOpenUserJS:masterfrom
.../scriptStorage.js#713Martii merged 1 commit intoOpenUserJS:masterfrom
Conversation
* Misplaced `var` declarations ... closer to current ES5.x implementation for some others just misplaced * Reordered on first use for all vars... not as critical but good for debugging * Changed `s3` early initialization... shouldn't need to be **and if it did** it should always be `NOTE:`d with a comment * Some newlines for white space usage... easier reading/debugging * Any `require` statements all at top... `require` functions become cached and always stay loaded after first use... so really shouldn't be attempting to optimize these in our code... only perceived benefit is startup pm time but is loaded elsewhere so no real benefit. * Some notation of `WARNING:` statement on incomplete error handling * White-space trim that my editor didn't catch on a previous commit even though it is on all the time everywhere. :\ * `WARNING:` note added for resaving script model object for no apparent reason... again should be notated. * Max line length of 100 conformance Applies with OpenUserJS#285, OpenUserJS#486, OpenUserJS#390, and OpenUserJS#19 ... perhaps some more... very minimal change in project behavior here which is why it is isolated.
Martii
added
the
CODE
Martii
added a commit
that referenced
this pull request
Aug 18, 2015
Some misc STYLEGUIDE.md conformance in `.../scriptStorage.js` Auto-merge
Member
Author
There was a problem hiding this comment.
@sizzlemctwizzle Cc: @Zren
Is there a particular reason why we are resaving the Script object here?
Seems to me this may be a logic issue if it is truly needed as the function name implies that it is just sendScript... possible migration artifact as well. e.g. dead code and/or undocumented migration point.
Contributor
There was a problem hiding this comment.
We need to save the script object it to increment the install counter.
Member
Author
There was a problem hiding this comment.
Ahh good point that I missed... different train of thought when STYLEGUIDE'ing... thanks.
Member
Author
There was a problem hiding this comment.
Although no error handling present if there is a connection failure... even if just internal notification.
Martii
pushed a commit
to Martii/OpenUserJS.org
that referenced
this pull request
Aug 19, 2015
* Clarified what this section of code is doing and not doing Post OpenUserJS#713 fix
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
vardeclarations ... closer to current ES5.x implementation for some... others just misplaceds3early initialization... shouldn't need to be and if it did it should always beNOTE:d with a commentrequirestatements all at top...requirefunctions become cached and always stay loaded after first use... so really shouldn't be attempting to optimize these in our code... only perceived benefit is startup pm time but is loaded elsewhere so no real benefit. (skipped one node native but may catch it later)WARNING:statement on incomplete error handlingWARNING:note added for resaving script model object for no apparent reason... again should be notated.Applies with #285, #486, #390, and #19 ... perhaps some more... very minimal change in project behavior here which is why it is isolated.