Skip to content

Conversation

@onlyurei
Copy link

@onlyurei onlyurei commented Dec 1, 2016

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

test/parallel/test-fs-append-files.js

  • var -> const / let
  • assert.equal -> assert.strictEqual

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@mscdex mscdex 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
@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

There should be a space after test: in the first line of the commit message.

test/parallel/test-fs-append-file.js
- var -> const / let
- assert.equal -> assert.strictEqual
@onlyurei
Copy link
Author

onlyurei commented Dec 1, 2016

@mscdex fixed.

@onlyurei onlyurei changed the title test:var to const/let; assert.equal to strictEqual test: var to const/let; equal to strictEqual Dec 1, 2016
@Trott
Copy link
Member

Trott commented Dec 16, 2016

CI failure is unrelated.

@Trott
Copy link
Member

Trott commented Dec 16, 2016

Looks like someone else came along and did these exact same changes in #10110 about 3 days later. Sorry about that @onlyurei!

However! If you still want to get some improvements in to this file, you can try again with a new pull request. One thing I notice is that there are a bunch of unnecessary fs.unlinkSync() calls at the end of the file. They are there to clean up temp files, but tests that use the temp directory remove it and recreate it so there's no need for tests to clean up that way.

Just making that change would be enough for a pull request. If you want to do something a little more involved (or if you want a good second commit after that one lands), all the ncallbacks stuff can be replaced with wrapping of callbacks in common.mustCall().

In the meantime, I'm going to close this pull request. Sorry it didn't land! Thanks for doing it, though!

@Trott Trott closed this Dec 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants