Alias res.sendfile as res.sendFile

[ForbesLindesay]

sendfile is the wrong casing, so I always get it wrong. Can we just alias it as res.sendFile?

[rmg]

FWIW, sendfile is more consistent with the Linux/BSD system call it is kind-of-sort-of emulating.

[ForbesLindesay]

hmm, I mostly develop on windows, so that was lost on me, but it seems more important to be consistent with JavaScript naming conventions than Linux/BSD naming conventions.

[sam-github]

Current naming is consistent with Node.js convention of keeping original name of underlying unix system call, see readlink. But I doubt express users spend a lot of time thinking unix system programming, and the naming looks pretty inconsistent to me, too. And having poked around a bit, I'm having trouble seeing if it even uses sendfile(2) (Node.js doesn't have a binding for that, for sure).

So, fwiw, I'm +1, if accompanied by a documentation change and unit test.

[rmg]

@ForbesLindesay sorry, I wasn't nearly clear enough. I was only trying to shed light on why it may have been all lower case to start with, it wasn't meant as an opinion on the change itself. :-)

[ForbesLindesay]

@rmg thanks for the clarification :-)

[coveralls]

Coverage remained the same when pulling d9a2b06 on ForbesLindesay:patch-1 into 1854a5d on strongloop:master.

[ForbesLindesay]

That's because I kept the existing sendfile alias, which was tested. I've updated the tests to check it works with both sendfile and sendFile.

[ritch]

I see where you are going with this, but the diff is ridiculous large for adding an alias. The current goal of this module and 5.x is to really move things out and to keep express very minimal. I understand how a change like this could be helpful, but right now its not helpful enough to justify the overhead (more docs, history, etc).

Thank you for the PR though, and if you are looking to contribute to express there are a lot of areas to help out on. Take a look at the ideas tag in this repo, or all the modules in here.

[ForbesLindesay]

@rmg that's because I put most of the body of a file inside a function, so that it can be called twice (with the two different aliases).

@ritch If it is the size of the diff that concerns you, there are things we can do about that. For example, if you just look at the commit that makes the change (https://github.com/ForbesLindesay/express/commit/aa9adf4a40339848b4972e4bcb0bbaa8782ea370) you'll see it is a single line.

[sindresorhus]

@ritch you should really look at the diff with whitespace ignored: https://github.com/strongloop/express/pull/2266/files?w=1

[ForbesLindesay]

Long term I would view this as a rename, but it seems un-necessary to break backwards compatibility.

[Fishrock123]

@sindresorhus there is a reason for the white space.

[dougwilson]

So, if I'm apparently required to make a statement on this issue, here it is:

I, myself, do not have a specific opinion on what the method should be named. There are arguments against keeping it the same or changing it. Though in my opinion, the argument that it is somehow wrong in JavaScript is weak, but the argument that it does not fit in with the naming of the other methods on req and res is compelling to me. Since a change like this would not fall until a 4.8 (following semver), there would be plenty of time to gauge what the community thought of the change to sway me one way or the other.

If it seems there was consensus to change the name, I would have done a few things here to lower the bar for contributions and because the PR is really about the idea, not to pick apart the commit:

1. The commit message and other pickyness doesn't follow what commits look like in the repo, so I would amend the commit to fix these
2. The tests not not particularly acceptable (existing tests should not be touched), so I would also amend that change into the commit
3. I would take the name change as an opportunity to actually implement res.sendfile does not accept file system paths #1906 in the new name, because there is no good way to make that an easy upgrade, but if it's going to change names, may as well make the new name implemented correctly in the first place (this is also not @ForbesLindesay 's job in my opinion)
4. I would debate on when the timing to add a deprecation message to the old name would be, because I'd have to think more on that and don't have that answer right now to write for everyone :)

[ritch]

@ForbesLindesay I closed this PR in haste. Sorry about that. Reopened.

If it is the size of the diff that concerns you, there are things we can do about that.

The size of the diff was just surprising. I guess I would expect an addtion similar to this and not much else.

I would debate on when the timing to add a deprecation message to the old name would be, because I'd have to think more on that and don't have that answer right now to write for everyone

@dougwilson how was this handled for app.del()? That seems like a similar change that seemed to work out well.

[coveralls]

Coverage remained the same when pulling d9a2b06 on ForbesLindesay:patch-1 into 1854a5d on strongloop:master.

[jeffbcross]

@ritch GH will remove whitespace changes from diff if you append /?w=1: https://github.com/strongloop/express/pull/2266/files?w=1

I'm not sure the conventions of this repo, but wouldn't it be sufficient to add a test that sendFile === sendfile instead of running the whole spec against both?

[Fishrock123]

@jeffbcross The whitespace is there for a reason. Also, I think sendFile === sendfile would be fine.

[jeffbcross]

I must have mis-spoke. I know the whitespace should be in the code, but it
is helpful for the reviewer to temporarily filter it from view in github.
On Aug 3, 2014 11:07 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

@jeffbcross https://github.com/jeffbcross The whitespace is there for a
reason. Also, I think sendFile === sendfile would be fine.

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