Make --require relative to where the command is run. #346
Conversation
ariporad
changed the title
invoked, not where the test is. Refs #343.
|
Ok, @vdemedes, @jamestalmage, @sindresorhus: This should be ready for real now. Sorry about that. |
| @@ -314,9 +314,14 @@ test('test file in node_modules is ignored', function (t) { | |||
| test('Node.js-style --require CLI argument', function (t) { | |||
| t.plan(1); | |||
|
|
|||
| var requirePath = path.relative(process.cwd(), path.join(__dirname, 'fixture', 'install-global.js')); | |||
| // `path` will use \\ to seperate paths on windows, but require doesn't. | |||
| requirePath = requirePath.split(path.sep).join('/'); | |||
There was a problem hiding this comment.
Shouldn't windows users be allowed to use \ as a path separator, since that is what they are used to?
Seems to me that
ava --require .\fixtures\install-global.js .\fixture\validate-installed-global.js
is the valid windows style alternative to
ava --require ./fixtures/install-global.js ./fixture/validate-installed-global.js
I would prefer it if AVA accepted either path separator and just normalized it internally.
There was a problem hiding this comment.
@jamestalmage: it's iffy, because node doesn't allow that (AFAIK), and 'require' doesn't allow that, so it would be a strange thing to do, but I'll add it if you really want.
There was a problem hiding this comment.
You can use \\ in require and --require, but it only works on Windows. So it won't be cross-platform. We could normalize internally, but do we really want to implement it differently than $ node --require?
There was a problem hiding this comment.
@sindresorhus: I'd say no, no you don't.
There was a problem hiding this comment.
I am not suggesting we change the implementation. Just the line here in the test that normalizes the path to posix. We don't need to do that. And we want the test to fail if it doesn't allow windows users to use it. resolveCwd is going to end up normalizing it for the given operating system anyways.
There was a problem hiding this comment.
@jamestalmage Oh, ok. I misunderstood. Agree with that.
@ariporad Can you update?
There was a problem hiding this comment.
@jamestalmage, @sindresorhus: Ok, I removed that line.
| @@ -9,6 +9,7 @@ var figures = require('figures'); | |||
| var globby = require('globby'); | |||
| var chalk = require('chalk'); | |||
| var fork = require('./lib/fork'); | |||
| var resolveCwd = require('resolve-cwd'); | |||
|
Test failing on Windows: https://ci.appveyor.com/project/sindresorhus/ava/build/609 |
|
@jamestalmage, @sindresorhus: Now this is doing what was happening before, require is throwing an error when you use |
|
Yeah, seems so. Tweaked it a little bit and landed. Thanks @ariporad :) |
|
@sindresorhus: Thanks! One more question if you don't mind: I rather like AVA, it's a lot more lightweight than mocha. But in looking at the issue tracker, and hacking the source, there are some things that I would like to see implemented (one small thing is renaming |
|
We appreciate you wanting to help out :D Small focused pull requests are easier to merge than large ones. For larger changes, open an issue or discuss in the Gitter chat beforehand. More here: https://github.com/sindresorhus/ava/blob/master/contributing.md#submitting-a-pull-request
@jamestalmage and @vdemedes can land pull requests too. Don't worry about that. |
👍 Though you may want to wait until we get or #352 and/or #349 merged. They both make significant modifications to In general, If you are embarking on something ambitious, you should always seek feedback/clarification by opening an issue or making sure there is consensus on an existing issue thread. Open a PR as soon as you have done the minimum amount of work to demonstrate your idea (just prefix the title with |
@jamestalmage Good advice. Can you add this to contributing.md? |
This Fixes #343.