Skip to content

fs: copyObject() had unused argument#10041

Closed
Ethan-Arrowood wants to merge 1 commit intonodejs:masterfrom
Ethan-Arrowood:ethan-test-fs
Closed

fs: copyObject() had unused argument#10041
Ethan-Arrowood wants to merge 1 commit intonodejs:masterfrom
Ethan-Arrowood:ethan-test-fs

Conversation

@Ethan-Arrowood
Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood commented Dec 1, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

The fs function copyObject() had two arguments:
source and target. On the first line of the function it
assigned the target variable to:
arguments.length >= 2 ? target : {};

The function copyObject() was not called directly by
any test, but it is called in other fs functions. When it
was called it was only ever called with a single argument,
source. Thus I have removed the target argument and assigned
it to an empty object like it was being assigned to in the
original ternary operator.

@Ethan-Arrowood
fs: copyObject() had unused argument: target
92a4bb8 
The fs function copyObject() had two arguments:
source and target. On the first line of the function it
assigned the target variable to:
arguments.length >= 2 ? target : {};

The function copyObject() was not called directly by
any test, but it is called in other fs functions. When it
was called it was only ever called with a single argument,
source. Thus I have removed the target argument and assigned
it to an empty object like it was being assigned to in the
original ternary operator.
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
cjihrig
cjihrig approved these changes Dec 3, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but cc @thefourtheye, who I think originally added this.

thefourtheye
thefourtheye approved these changes Dec 3, 2016
View reviewed changes
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

The intention was to optionally pass an object and the properties were copied on to it. But, after several iterations of the review, we decided to go without the second argument. Original PR: #7831 Thanks @Ethan-Arrowood

@silverwind silverwind mentioned this pull request Dec 5, 2016
Closed
@Trott
Copy link
Member

Trott commented Dec 8, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/5328/

@Trott
Copy link
Member

Trott commented Dec 9, 2016

Hmmm...that CI run is a bit of a CI-is-having-issues run. Looks like Jenkins got a reboot or something, so let's try again.

CI: https://ci.nodejs.org/job/node-test-pull-request/5331/

@lpinca
Copy link
Member

lpinca commented Dec 14, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/5401/

lpinca pushed a commit to lpinca/node that referenced this pull request Dec 14, 2016
@Ethan-Arrowood @lpinca
fs: remove unused argument from copyObject()
e432c2b 
The fs function copyObject() had two arguments:
source and target. On the first line of the function it
assigned the target variable to:
arguments.length >= 2 ? target : {};

The function copyObject() was not called directly by
any test, but it is called in other fs functions. When it
was called it was only ever called with a single argument,
source. Thus I have removed the target argument and assigned
it to an empty object like it was being assigned to in the
original ternary operator.

PR-URL: nodejs#10041
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@lpinca
Copy link
Member

lpinca commented Dec 14, 2016

Landed in e432c2b.

Thanks!

@lpinca lpinca closed this Dec 14, 2016
MylesBorins pushed a commit that referenced this pull request Dec 15, 2016
@Ethan-Arrowood @MylesBorins
fs: remove unused argument from copyObject()
2673be6 
The fs function copyObject() had two arguments:
source and target. On the first line of the function it
assigned the target variable to:
arguments.length >= 2 ? target : {};

The function copyObject() was not called directly by
any test, but it is called in other fs functions. When it
was called it was only ever called with a single argument,
source. Thus I have removed the target argument and assigned
it to an empty object like it was being assigned to in the
original ternary operator.

PR-URL: #10041
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
Merged
italoacasas pushed a commit that referenced this pull request Dec 15, 2016
2016-12-20, Version 7.3.0 (Current)
617dbf7 
Notable changes

SEMVER-MINOR
- url:
    - add inspect function to TupleOrigin (Safia Abdalla) #10039
- crypto:
    - allow adding extra certs to well-known CAs (Sam Roberts) #9139

SEMVER-PATCH
- buffer:
    - fix single-character string filling (Anna Henningsen) #9837
    - handle UCS2 .fill() properly on BE (Anna Henningsen) #9837
- url:
    - including base argument in originFor (joyeecheung) #10021
    - improve URLSearchParams spec compliance (Timothy Gu) #9484
- http:
    - remove stale timeout listeners (Karl Böhlmark) #9440
- build:
    - fix node_g target (Daniel Bevenius) #10153
- fs:
    - remove unused argument from copyObject() (Ethan Arrowood) #10041
- timers:
    - fix handling of cleared immediates (hveldstra) #9759

PR-URL: #10277
italoacasas pushed a commit that referenced this pull request Dec 17, 2016
2016-12-20, Version 7.3.0 (Current)
0afa836 
Notable changes:
* **crypto**:
  - Allow adding extra certificates to well-known CAs. (Sam Roberts)
    [#9139](#9139)
* **buffer**:
  - Fix single-character string filling. (Anna Henningsen)
    [#9837](#9837)
  - Handle UCS2 `.fill()` properly on BE. (Anna Henningsen)
    [#9837](#9837)
* **url**:
  - Add inspect function to TupleOrigin. (Safia Abdalla)
    [#10039](#10039)
  - Including base argument in originFor. (joyeecheung)
    [#10021](#10021)
  - Improve URLSearchParams spec compliance. (Timothy Gu)
    [#9484](#9484)
* **http**:
  - Remove stale timeout listeners. (Karl Böhlmark)
    [#9440](#9440)
* **build**:
  - Fix node_g target. (Daniel Bevenius)
    [#10153](#10153)
* **fs**:
  - Remove unused argument from copyObject(). (EthanArrowood)
    [#10041](#10041)
* **timers**:
  - Fix handling of cleared immediates. (hveldstra)
    [#9759](#9759)
* **src**:
  - Add wrapper for process.emitWarning(). (SamRoberts)
    [#9139](#9139)
  - Fix string format mistake for 32 bit node.(Alex Newman)
    [#10082](#10082)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@thefourtheye thefourtheye thefourtheye approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Ethan-Arrowood @Trott @lpinca @jasnell @thefourtheye @cjihrig @MylesBorins @imyller @nodejs-github-bot