Remove res.json(status, obj) signature #2939
Conversation
| @@ -24,6 +24,7 @@ This is the first Express 5.0 alpha release, based off 4.10.1. | |||
| - `req.acceptsEncoding` - use `req.acceptsEncodings` | |||
| - `req.acceptsLanguage` - use `req.acceptsLanguages` | |||
| - `res.json(obj, status)` signature - use `res.json(status, obj)` | |||
There was a problem hiding this comment.
should this existing line be rewritten to:
- `res.json(obj, status)` signature - use `res.status(status).json(obj)`
?
There was a problem hiding this comment.
No, because that is the change log for the first alpha. You can't changed what has already been released.
tunniclm
force-pushed
the
remove_deprecated_res_json_status_obj_sig
branch
from
a720b1e
to
7b7e995
Compare
|
Fixed the History.md mistake. |
tunniclm
force-pushed
the
remove_deprecated_res_json_status_obj_sig
branch
from
7b7e995
to
2ba80f4
Compare
| @@ -1,3 +1,9 @@ | |||
| unreleased | |||
There was a problem hiding this comment.
@dougwilson is this how we update History.md for unreleased changes to the codebase?
There was a problem hiding this comment.
Please do it the same as all the previous commits have done it in the 5.0 branch.
There was a problem hiding this comment.
@dougwilson It's a little confusing just looking at the commits on 5.0 branch. Some have no update to History.md (eg: 8a387d3), some are against a named release like 4.13.4 / 2016-01-21 (eg: 193bed2), some are against 5.x (eg: 6343288), some are against unreleased (eg: 6847405).
I guess it is supposed to be 5.x? You have to look quite a long way back to find a commit with that, since many commits looks to be merges in from the master branch / 4.x release using unreleased.
Please confirm the correct title.
There was a problem hiding this comment.
Hi @tunniclm, you want to look only at the commits made on the 5.0 branch, not al the commits merged into there. You can use the git commands to walk only the left-hand of merges to just see commits that were committed against a branch rather than looking at the really confusing GitHub smashed view.
When you open each one of those commits, GitHub will even show what branch that commit was against in the header.
There was a problem hiding this comment.
@dougwilson Something like git log -p --left-only while on branch 5.0?
|
@dougwilson this PR (and the other PRs by @tunniclm removing the deprecated signatures with status code) looks fine to me, except for the uncertainty about the content in the History.md. What do you think? |
| // settings | ||
| var app = this.app; | ||
| var replacer = app.get('json replacer'); | ||
| var spaces = app.get('json spaces'); | ||
| var body = JSON.stringify(val, replacer, spaces); | ||
| var body = JSON.stringify(obj, replacer, spaces); |
There was a problem hiding this comment.
nit: make the variable val, since we accept any value; it was only obj in the arguments so we could use val throughout the body without being hit by an argument reassignment deopt.
tunniclm
force-pushed
the
remove_deprecated_res_json_status_obj_sig
branch
2 times, most recently
from
4cb3f99
to
88df89f
Compare
|
Updated to address comments. |
tunniclm
force-pushed
the
remove_deprecated_res_json_status_obj_sig
branch
from
88df89f
to
a38a21c
Compare
|
@tunniclm coverage has decreased by a fraction, otherwise looks good to me. |
|
The reduction in coverage occurs because the number of lines in the file has reduced and coverage on that file was not 100% to begin with -- for response.js the coverage was 310/311, that is: 1 line was not covered; after the change the coverage is 305/306, still with 1 line not covered. The coverage on the changed function is 100%, it's just the 1 line that isn't covered in the file is now a greater percentage of the total number of lines. |
|
@dougwilson what do we in such situations? And can we land this? |
|
What is the line that is not covered? Can we not make a plan or pull request to get that line covered? Otherwise it will never get fixed. If we land that fix first, then all these will not have decreased coverage. |
|
I've submitted a pull request that adds coverage to that one line: #2986 |
|
@LinusU Great, that saves me a job this morning. |
|
Lets cherry-pick it when it lands? I have a feeling I might get some comments on pr so lets iron those out first 👍 |
|
The test PR targeting 4.x makes the most sense, since the coverage is missing there as well and we may end up in the same situation. The 4.x line gets merged into 5.x after each 4.x release or when needed, so that should automatically get it in 5.x based on procedure :) |
tunniclm
force-pushed
the
remove_deprecated_res_json_status_obj_sig
branch
from
a38a21c
to
d20aa40
Compare
|
Merged into 5.0 branch :) |
No description provided.