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 back semicolons to the HTTP server example in about page #656

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Apr 17, 2016

Reverts #641

Not sure what are the better practices there, I guess the purpose was to test the bot.
Anyway I'm filing this pr because semicolons have been removed and this makes the example inconsistent with all the others (guides, API docs, etc).

cc: @Fishrock123

@Fishrock123
Copy link
Contributor

Fishrock123 commented Apr 17, 2016

Feel free to re-add the semicolons but the rest is actually better practice, I.e. Not using writeHead() directly

@lpinca
Copy link
Member Author

lpinca commented Apr 17, 2016

Forgive my ignorance but what's wrong with writeHead()? It's a public api afaik.

@Fishrock123
Copy link
Contributor

Nothing is wrong with it, but chances are you are not going to be calling it directly from user code. :)

@lpinca lpinca changed the title Revert "About: update example to use better practices" Add back semicolons to the HTTP server example in about page Apr 18, 2016
@lpinca
Copy link
Member Author

lpinca commented Apr 18, 2016

I'm still not sure why I should prefer

res.statusCode = 200;
res.setHeader('Content-Type', 'text/plain');

over

res.writeHead(200, { 'Content-Type': 'text/plain' });

but it doesn't matter. I've updated the pr to only re-add the semicolons.

@fhemberger fhemberger merged commit a0cd761 into master Apr 18, 2016
@fhemberger fhemberger deleted the revert-641-semicolonless branch April 18, 2016 16:42
@fhemberger
Copy link
Contributor

Cool, thanks!

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.

3 participants