This fixes cases where apostrophes are present in pathnames

[ronkorving]

Fixes #214

Do not merge this (yet). I've not been able to test this, because I cannot for the love of me get a test environment up and running. Whenever I install ghooks using either:

• npm i --save-dev ../ghooks
• npm i --save-dev ronkorving/ghooks#allow-apostrophes

I get installation errors:

> [email protected] install /Users/ronkorving/Projects/ghooks'test/node_modules/ghooks
> node ./bin/module-install

(node:21927) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Cannot find module '../dist/install'
(node:21927) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
npm WARN ghooks'[email protected] No description
npm WARN ghooks'[email protected] No repository field.

Do you have any advice on how I should test this project?

[ta2edchimp]

Hi Ron,

running npm install (--save-dev) ghooks should work — npm will install the published module from it's registry. This one includes a folder named dist in which the transpiled code lies.

npm installing from a repo should find the folder dist to not exist and transpile from sources using babel, executing the resulting file dist/install.js afterwards. Thus failing with the message Cannot find module '../dist/install'.
It seems to me, the folder dist is existing within /Users/ronkorving/Projects/ghooks'test/node_modules/ghooks, but is empty.

Could you please verify whether the folder exists and is empty?
And if it is so, could you please just delete that empty folder and try again?

If npm installing fails again afterwards, it may read something like Error: Command failed: npm run build and sh: babel: command not found which indicates that upon installation, the dist folder cannot be found and ghooks tries to transpile itself via babel. And that may fail with babel being not present in your test project setup.
In that case, you could add babel and the related dependencies (listed in ghooks' own package.json under devDependencies) and npm install your fork again.

But I would recommend, you add a test case within your fork of the ghooks repository. (That is, add a test case that can be run by the test runner to the file test/hook.template.test.js).
If there's something I can help you with regarding that, please just give me a hint (a response from my end could take some time, I'm EU timezone).

[codecov-io]

Codecov Report

Merging #216 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #216   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          96     96           
  Branches        4      4           
=====================================
  Hits           96     96

Impacted Files	Coverage Δ
lib/hook.template.js	100% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9129a3...0182eaf. Read the comment docs.

[ronkorving]

The node_modules/ghooks/dist folder exists, and is empty.

I removed the folder, then wanted to "try again", but wasn't sure what you wanted me to try again, so I ran both npm build and npm install, but that didn't change anything.

Then I went into node_modules/ghooks and ran npm install from there. A long build process later, I have a dist folder again, and it is still empty.

Copying my entire custom ghooks repo over to node_modules/ghooks, dropping .git from it and then running npm install from there seems to do the trick. The PR as it stands seems to work, as it generates:

var nodeModulesPath = '/Users/ronkorving/Projects/ghooks\'test/node_modules';

I definitely want to add a unit test for this feature, but the suite is a bit cryptic (sorry, I'm not very used to the tools used in ghooks, like sinon) so it might take me some time. That's why I wanted to start out with a real-world scenario. Is there a test that depends on path format that I could base something of?

[ta2edchimp]

Yeah, I am too unhappy about the transpilation step, currently requiring that build process at all. I'm pretty sure we would be able to avoid it in an upcoming release.

---

I think the line you already checked visually is exactly what a corresponding test should look for.

The according test should be put into test/hook.template.test.js, as that file contains the template for the hooks to be generated and thus covers the functionality that has been altered with this PR.
The test currently requires the hook.template (line 4) and interprets its result, at the moment only whether the generated message is rendered into a comment in the file's head is tested (7).

I would suggest — before requiring the hook.template — to manipulate the result of the called method process.cwd to be a made up path, including the character to be properly escaped by this PR's change. Something like /some/path/made/up/in/one's/own/mind/.
It should then be a small encapsulated test placed right after the test that already exists.

Of course, when overwriting process.cwd (because of its return value), a reference to the original function should be saved before to be restored after the tests run. (This is a nice reference about how to achieve this using mocha, others, like ava and the like, use similar means.)

So, putting all together, eventually test/hook.template.test.js should:

• describe tests for hook.template, that
  • before testing, stores a reference to the original process.cwd method in a variable and replaces process.cwd with a small function only returning a made up path
  • tests the generated_message token gets replaced as expected,
  • tests the line you already tested visually includes the made up path^note as expected,
  • after testing, restores the reference to the original process.cwd function.

Note: the made up path will be one level higher, as we manipulate the process.cwd result using join in the hook.template.

[gtramontina]

Hey there @ronkorving, first off, thank you for taking a stab at this issue! 🍻
Would you mind rebasing, given we are not transpiling anymore due to #218?

Cheers!

[ronkorving]

Very, very, happily :) I missed that PR going in, great news!

update: done

[gtramontina]

Thanks!! Would you mind adding a test scenario like @ta2edchimp described above? I think it is important for us to capture this as a test so we can prevent regression issues.

[ronkorving]

I agree 👍 I hope I'll get around to it soon.

[ta2edchimp]

Awesome.
Just drop us a line here, should any questions arise.