rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
381 stars 20 forks source link

Customized install prefix #42

Closed gabyx closed 5 years ago

gabyx commented 5 years ago

40

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 304


Changes Missing Coverage Covered Lines Changed/Added Lines %
uninstall.sh 17 19 89.47%
install.sh 20 25 80.0%
base-template.sh 8 14 57.14%
<!-- Total: 70 83 84.34% -->
Totals Coverage Status
Change from base Build 301: -0.3%
Covered Lines: 1676
Relevant Lines: 1867

💛 - Coveralls
gabyx commented 5 years ago

Can I also change : githooks.previous.searchdir to githooks.previous.searchDir. Potentially removing the old variable in uninstall.sh?

rycus86 commented 5 years ago

Can I also change : githooks.previous.searchdir to githooks.previous.searchDir. Potentially removing the old variable in uninstall.sh?

I'd leave it as it is, so it keeps working during updates.

gabyx commented 5 years ago

This also adresses how you provide updates when we provide a new version. We dont have a version number. Every mistake :-) say typo, we make in the config is hard to correct =).

Tests are noe failing where they should. The test-rule does not work yet, since the cli.sh help does crash since the install dir is not set. I have to make this more agnostic... Maybe make the help Fail as early as possible is a good thing... Testing everywhere if the install dir is feasible is cumbersome and only puts weight on the performance...

gabyx commented 5 years ago

Ok, I made the cli.sh a bit more agnostic. First handling the help case, and afterwards the install directory is checked if its missing -> abort.

gabyx commented 5 years ago

We still need to tweak the tests which do not install githooks.

gabyx commented 5 years ago

Ok, now I think its ready for another review.

rycus86 commented 5 years ago

It would be awesome to have some tests for these new code paths as well. :pray:

gabyx commented 5 years ago

Could we mock all tests with a default install directory and test this... :-)

gabyx commented 5 years ago

All review changes included. Tests 108 added plus a whole run with custom installation prefix test-installprefix.sh

rycus86 commented 5 years ago

My main concern is that I still don't want any error printed or failure if the install dir is not set, in that case everything should keep working as they are today, with the default path pointing to ~/.githooks. On testing: all existing tests should keep working without any changes, plus we need one test for the new codepaths (maybe one for install, one for base-template and one for cli). We don't really need a whole new run with all installs going with a prefix, the existing ones should be fine for when we don't, and a handful of new ones can check the --prefix case.

rycus86 commented 5 years ago

On a side note: I'll be away from laptops for a few days, so I can only merge this in in the second half of next week. I'll try to still review PR changes on my mobile. Thanks!

gabyx commented 5 years ago

For the default: I can do this for the cli.sh its already defaulting, I think for the basetemplate too, both emit a warning, which I would change to your suggestion above.

Regarding the tests: I set the configs only to ignore the warning, so with your changes I can drop these. The complete new test run was a good help to catch stupid errors, there were a hand full of these. I would leave it in. The tests have some changes with respect to detecting where it was installed. I gonna make some changes again.

gabyx commented 5 years ago

It was far easier to make a whole new run instead of adding only some new ones, I will take a lool

gabyx commented 5 years ago

On WSL the config is a duplikation with corrected paths

gabyx commented 5 years ago

I made the changes you suggested :)

gabyx commented 5 years ago

Done.

gabyx commented 5 years ago

I think it should be mergable now :-)

rycus86 commented 5 years ago

I think it should be mergable now :-)

There are still some test changes to revert, please.

gabyx commented 5 years ago

Ok, these reverts would not have been necessary... Its done.

rycus86 commented 5 years ago

Looks good, just some minor things to fix up - I'll sort them out after merging. Thanks!