Skip to content

Conversation

seabrookmx
Copy link
Contributor

Import fixturesDir path from common/fixturesDir
module rather than from common.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
@Trott
Copy link
Member

Trott commented Oct 8, 2017

Hi, @seabrookmx! Welcome and thanks for this! Can you update this to use fixtures.path()? It's documented at https://github.com/nodejs/node/tree/master/test/common#fixtures-module

Copy link
Member

Choose a reason for hiding this comment

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

There is a dedicated fixtures.path function that could be used here.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 9, 2017

Ah, sorry @Trott I did not read your comment before writing mine.

Import fixturesDir path from common/fixturesDir
module rather than from common.
@seabrookmx
Copy link
Contributor Author

@Trott Thanks for the link. Commit updated 🙇‍♂️

@gireeshpunathil
Copy link
Member

@seabrookmx - thanks, can you also apply @BridgeAR's recommendation to improve the code further?


const path = require('path');
const file = path.resolve(common.fixturesDir, 'x1024.txt');
const file = path.resolve(fixtures.path(), 'x1024.txt');
Copy link
Member

@lpinca lpinca Oct 13, 2017

Choose a reason for hiding this comment

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

This could be just fixtures.path('x1024.txt').

@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

@joyeecheung
Copy link
Member

Landed in bb56fbb, thank you for the contribution!

joyeecheung pushed a commit that referenced this pull request Oct 14, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: nodejs/node#15887
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
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.

10 participants