Skip to content

fix several examples #3310

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

Closed
hiowenluke wants to merge 4 commits into from
Closed

fix several examples #3310

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
262ba10
fix several examples
hiowenluke May 17, 2017
7a02b1e
remove missing.md
hiowenluke May 17, 2017
42b40f7
fix typo
hiowenluke May 17, 2017
48a281c
update the comments for installing redis
hiowenluke May 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions examples/online/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
// first:
// $ npm install redis online
// first (if you have not done it yet):
// $ npm install redis
// $ brew install redis
Copy link
Contributor

@dougwilson dougwilson May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If install instructions are added, can they be more accessible to the general user base? For example, this only works if the user is both (a) using MacOS and (b) has Homebrew installed (not of which is noted in here, just like how installing Redis was not noted before). Perhaps remove the command altogether and just add a link to Redis's site for how to install?

Copy link
Contributor Author

@hiowenluke hiowenluke May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, Doug.

Copy link
Member

@wesleytodd wesleytodd May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I agree with adding this. In an example the point is to show express, the assumption being you already know you need a redis instance running somewhere. I guess I might be fine with saying ensure you have redis running on its default port, since it does not assume osx and brew.

Copy link
Member

@wesleytodd wesleytodd May 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry @dougwilson your messages came in after I added mine :)

// $ redis-server

// then:
// $ npm install online

/**
* Module dependencies.
*/
Expand Down
2 changes: 1 addition & 1 deletion examples/params/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ app.get('/users/:from-:to', function(req, res, next){
var from = req.params.from;
var to = req.params.to;
var names = users.map(function(user){ return user.name; });
res.send('users ' + names.slice(from, to).join(', '));
res.send('users ' + names.slice(from, to + 1).join(', '));
});

/* istanbul ignore next */
Expand Down
2 changes: 1 addition & 1 deletion examples/route-separation/views/index.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

<ul>
<li>Visit the <a href="/users">users</a> page.</li>
<li>Visit the <a href="/users">posts</a> page.</li>
<li>Visit the <a href="/posts">posts</a> page.</li>
</ul>

<% include footer %>
3 changes: 2 additions & 1 deletion examples/search/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// first:
// first (if you have not done it yet):
// $ npm install redis
// $ brew install redis
Copy link
Contributor

@dougwilson dougwilson May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same Redis install instructions issue.

// $ redis-server

/**
Expand Down
3 changes: 2 additions & 1 deletion examples/session/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// first:
// first (if you have not done it yet):
// $ npm install redis
// $ brew install redis
Copy link
Contributor

@dougwilson dougwilson May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same Redis install instructions issue.

// $ redis-server

var express = require('../..');
Expand Down
2 changes: 1 addition & 1 deletion examples/view-constructor/github-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Module dependencies.
*/

var http = require('http');
var https = require('https');
var path = require('path');
var extname = path.extname;

Expand Down