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

Add Node's path module #3046

Merged
merged 2 commits into from
Jul 31, 2016
Merged

Add Node's path module #3046

merged 2 commits into from
Jul 31, 2016

Conversation

hamzaOp
Copy link
Contributor

@hamzaOp hamzaOp commented Jul 30, 2016

Add Node's path module to the static-files example.

@hamzaOp hamzaOp changed the title add Node's path module Add Node's path module Jul 30, 2016
@blakeembrey
Copy link
Member

Seems like a useful change for people that don't realise it's how you should be doing it. However, can you avoid unnecessary changes? Just make it path.join inline instead of using variables, and avoid the new additions. I'd also follow the existing code style, which is to have a space after the comma between arguments.

@hamzaOp
Copy link
Contributor Author

hamzaOp commented Jul 31, 2016

Thanks for the feedback :) , actually this is my first contribution so I'm still learning how to improve the code properly.

@blakeembrey
Copy link
Member

Awesome! It looks great now 😄

FWIW, you can also do path.join(__dirname, 'public/css'). Either way, I'm happy to merge this change!

@blakeembrey blakeembrey merged commit 3c54220 into expressjs:master Jul 31, 2016
@hamzaOp
Copy link
Contributor Author

hamzaOp commented Jul 31, 2016

Thank you 😄

mykiimike added a commit to mykiimike/express that referenced this pull request Aug 15, 2016
@dougwilson
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants