Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement: default pattern #9

Merged
merged 7 commits into from
Jan 19, 2015
Merged

implement: default pattern #9

merged 7 commits into from
Jan 19, 2015

Conversation

azu
Copy link
Member

@azu azu commented Jan 19, 2015

ref #8

If the user didn't set directories.test, use 'test/**/*.js' as default pattern.

@azu azu changed the title implement: default patterns implement: default pattern Jan 19, 2015
This was referenced Jan 19, 2015
@yosuke-furukawa
Copy link
Member

僕のコミットをムダにしないために、いくつかレビューコメント残しておきます。

@twada
Copy link
Member

twada commented Jan 19, 2015

@yosuke-furukawa++

for (var i = 0; i < paths.length; ++i) {
var dir = pather.dirname(paths[i]);
var dirName = dir.split(pather.sep).pop();
if (dirName !== packageName && fs.existsSync(pather.join(dir, 'package.json'))) {
Copy link
Member

Choose a reason for hiding this comment

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

existsSync is deprecated near the future.
if the package.json is not found, you don't require in line 6.
so the check is meaningless.

EDIT: sorry, scratch that. I misunderstood this specification.
However existsSync is not needed.

Copy link
Member

Choose a reason for hiding this comment

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

面倒なので日本語で。
v0.10とかだとaccessSyncもないから今のところはexsitsSyncでいいような気がしてきました。
真面目にやるならこうですかね。

var existsSync = fs.accessSync ? fs.accessSync : fs.existsSync;

// ...

 if (dirName !== packageName && existsSync(pather.join(dir, 'package.json'))) {
  // ...
}

see: nodejs/node#257

まぁまだiojsでもexistsSyncはdeprecatedにはなっていないので、参考までに。

typeof pkg.directories.test === 'string') {
return normalizeDir(pkg.directories.test);
}
return "test/"; // default value
Copy link
Member

Choose a reason for hiding this comment

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

I think default value should not define in this function.

  • before
function getTestDirFromPkg(pkg) {
    if (pkg &&
        typeof pkg.directories === 'object' &&
        typeof pkg.directories.test === 'string') {
        return normalizeDir(pkg.directories.test);
    }
    return "test/";
}
  • after
var defaultTestDir = "test/";
function getTestDirFromPkg(pkg) {
    if (pkg &&
        typeof pkg.directories === 'object' &&
        typeof pkg.directories.test === 'string') {
        return normalizeDir(pkg.directories.test);
    }
    return defaultTestDir;
}

var assert = require("assert");
var getTestDir = require("../lib/get-test-dir");
describe("get-test-dir-test", function () {
context("when `pkg.directories.test` is not found", function () {
Copy link
Member

Choose a reason for hiding this comment

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

add a test case when pkg.directories is undefined.

@azu
Copy link
Member Author

azu commented Jan 19, 2015

@yosuke-furukawa Thanks for review! I'v fixed.

@yosuke-furukawa
Copy link
Member

Thank you for the quick fixing!!

azu added a commit that referenced this pull request Jan 19, 2015
implement: default pattern

fix #8
@azu azu merged commit e1c53f8 into master Jan 19, 2015
@azu azu deleted the defaults-pattern branch January 19, 2015 07:09
@azu azu mentioned this pull request Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants