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.

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

some code clean up #2004

Merged
merged 1 commit into from Mar 25, 2014
Merged

some code clean up #2004

merged 1 commit into from Mar 25, 2014

Conversation

agchou
Copy link
Contributor

@agchou agchou commented Mar 25, 2014

Hey guys!

First of all, just want to say thank you for creating such an amazing framework. I'm currently working hard to become a full-stack javascript developer and have been recently learning express.

While reading through the code many times, I noticed some inconsistencies in the formatting (especially with the "Module Dependencies") and thought it could be a chance for me to try and become involved in open source. In this commit I formatted the "Module Dependencies", moved the export to the top with comments, and added comments to the initialize function for Layer.

If this is not acceptable please let me know if I can help out in any other way. I am willing to go through all files and format "Module Dependencies" or any other mundane tasks you need completed. Thanks so much for reading—I am greatly inspired by you all!

@jonathanong
Copy link
Member

yeah this is how TJ writes code, but roman and i don't really follow this paradigm. you're just repeating the code without explaining any "why"s. if it made the code any more understandable (i.e. explanations), then i'd merge it, but it's just stylistic right now so i'm meh. consistency is nice though

@agchou
Copy link
Contributor Author

agchou commented Mar 25, 2014

I see and definitely agree about the comment for initializing. If you want me to go through and change for consistency I will gladly.

@agchou
Copy link
Contributor Author

agchou commented Mar 25, 2014

Removed Layer initialization comment I added. Added a few missing semicolons, removed unnecessary spacing, and changed module dependencies to be consistent.

@jonathanong
Copy link
Member

@defunctzombie do you like single vars or multiple vars? i like multiple vars because of easier diffs and var can be typed with your left hand while your right hand does other things.

@defunctzombie
Copy link
Contributor

I'm a multiple var kind of guy but I typically remain consistent with the project, just messed up with this file :)

@jonathanong
Copy link
Member

i think TJ switched to multiple vars as well. i've been writing code as multiple vars as i change stuff, but i didn't want to go through every file and switch it.

@agchou do you want to switch the style to multiple vars? i'm not sure how big the diff is, but if it's too big, multiple vars is fine. consistency ftw!

@agchou
Copy link
Contributor Author

agchou commented Mar 25, 2014

@jonathanong yah I'll switch it to multiple vars as long as @defunctzombie and other are cool with it. Just to clarify I am going to switch files from single var (many commas) to multiple vars (1 per declaration).

@agchou
Copy link
Contributor Author

agchou commented Mar 25, 2014

Let me know if you want me to go through any of the other directories (examples, test, etc.). Thanks for giving me the opportunity to help out!

@jonathanong
Copy link
Member

looks good. we can merge this PR for now and feel free to open up more PRs.

can you squash the commits?

@agchou
Copy link
Contributor Author

agchou commented Mar 25, 2014

@jonathanong done! will keep chugging away when I have time to go through the remaining files.

btw git is probably the most annoying thing I've encountered while learning software engineering... why, just why.

@jonathanong
Copy link
Member

it's annoying at frist but it all makes sense when you understand it

jonathanong added a commit that referenced this pull request Mar 25, 2014
@jonathanong jonathanong merged commit 0120874 into expressjs:master Mar 25, 2014
rlidwka pushed a commit to rlidwka/express that referenced this pull request Aug 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants