Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 264prefix a parm lists #267

Merged
merged 7 commits into from
Jul 24, 2014
Merged

Issue 264prefix a parm lists #267

merged 7 commits into from
Jul 24, 2014

Conversation

Martii
Copy link
Member

@Martii Martii commented Jul 16, 2014

See the plethera of line notes and commit messages.

Applies to #264 and inclusive to #262

Any further changes related to #264 we can catch on the fly I think.

Assigning to sizzle for final testing although others are welcome to test as well.

Martii added 7 commits July 14, 2014 15:51
Normalize on a prefix for function parameters
d7465a3 
* Normalize `callback` and `cb` to `aCb`

Not finished yet but shown for @Zren to see what I'm doing

Applies to OpenUserJS#264
Use full naming for aCallback
29e007b 
Applies to OpenUserJS#264 and d7465a3#commitcomment-7004333

> You don't use short forms (cb = callback) with verbose varible name standards.

Not explicitly declared the STYLEGUIDE** but can be used to accomodate those who don't know what the short term of a callback is... been debating on making a minimum length too. e.g. not one letter identfiers with a potential prefix.
* At least 2 potential globals found
dd9a807 
* At least 1 redundant/unreachable statement found

Applies to OpenUserJS#262

Also some uncaught OpenUserJS#255 because no code was executed before... but not doing all of these yet.
Controllers completed, I believe, and some testing on dev... still ha…
af46e15 
…ve libs to do.

* Changed "WARNING" on login page to "CAUTION"... a little too assertive... some users may want their email addy in there.
* Notated some STYLEGUIDE conformance needs
* Notated very short function name(s)
* At least one undefined identifier
Should be done with minimal breakage... testing phase now.
82dc007 
Applies to OpenUserJS#264
More for OpenUserJS#262
df94b62 
* Change/Add some more comments in for Ambiguous
* Remove some comments added as it appears they have been identified.
* Unified what comments I found that I added for 264.

Applies to OpenUserJS#264 as well
Remainder of comments added normalized for searching
e9281dd 
* Removed one comment

Applies to OpenUserJS#264 and inclusive to OpenUserJS#262
@Martii Martii added this to the #262 milestone Jul 16, 2014
@Martii Martii mentioned this pull request Jul 18, 2014
Merged
@Zren
Copy link
Contributor

Zren commented Jul 23, 2014

This getting merged or what?

@sizzlemctwizzle
Copy link
Member

Yes. I'll merge it today.
On Jul 23, 2014 7:01 AM, "Chris Holland" [email protected] wrote:

This getting merged or what?

—
Reply to this email directly or view it on GitHub
#267 (comment)
.

sizzlemctwizzle added a commit that referenced this pull request Jul 24, 2014
@sizzlemctwizzle
Merge pull request #267 from Martii/Issue-264prefixAParmLists
ad76b37 
Issue 264prefix `a` parm lists
@sizzlemctwizzle sizzlemctwizzle merged commit ad76b37 into OpenUserJS:master Jul 24, 2014
@Martii Martii deleted the Issue-264prefixAParmLists branch July 24, 2014 03:37
@Martii Martii mentioned this pull request Jul 24, 2014
Closed
@Martii
Copy link
Member Author

Martii commented Jul 24, 2014

@sizzlemctwizzle
Somewhere between blissful missing and probably a subconscious motivation /controllers/auth.js was entirely missed (See #273). :\ Looking at it though it is still rather vague on what to change (tacking our data onto some other projects object isn't usually a good thing) ... and would appreciate you having a look at it to normalize to the standard... it's pretty short to change as long as it's correct. Thanks. I have no doubt in my mind that I may have missed a few other small ones too but we can fix on those when they crop up.

@sizzlemctwizzle
Copy link
Member

(tacking our data onto some other projects object isn't usually a good thing) ...

It's convoluted because we're using the Passport library in a way that was never intended. It's basically just one giant hack. There's probably a better way to do it, but I'd really have to mess around with it to clean it up.

@Martii Martii mentioned this pull request Jul 26, 2014
Closed
@jerone jerone mentioned this pull request Nov 29, 2014
Merged
@Martii Martii removed the needs testing Anyone can add this but it is primarily there for the Assignee indicating that Testers are wanted. label Feb 7, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. migration Use this to indicate that it may apply to an existing or announced migration.
Milestone
#262
Development

Successfully merging this pull request may close these issues.
4 participants
@Martii @Zren @sizzlemctwizzle @cletusc