git-validate icon indicating copy to clipboard operation
git-validate copied to clipboard

utils.configureHook breaking change in 2.2.1

Open pipedrive-bot opened this issue 9 years ago • 3 comments

Although the use case is very specific, the change of adding overwrite flag in configureHook was a breaking one.

Before the change this worked:

Validate.configureHook(hook, data, root);

However in #54 another parameter was added to the signature, without backwards compatibility. So for this case, overwrite flag now has to be specified everywhere it’s used.

In addition - as for the consistency – the overwrite flag is not specified the same way as with Validate.installScript() or Validate.copy() methods.

pipedrive-bot avatar Oct 06 '16 09:10 pipedrive-bot

i didn't determine changing an undocumented feature that was purposed for internal use only as a breaking change. users are not meant to pass the root parameter.

i am curious though, what's your use case for passing it? if it seems like it could be useful to more people i'm open to making it a documented parameter, which would prevent this sort of thing in the future.

i agree it's a little funky that the overwrite flag is not passed the same way as it is for copy and installScript though, that's something i'll change (and will be a major release so would resolve your complaint here as well).

nlf avatar Oct 06 '16 16:10 nlf

Sorry for delayed response, but here’s how we use your library.

In our project we’re using git-validate as the core for our own git-hooks repo. The idea is that the configurations are specified in our repository, with different linting tools and other helpers. And then our tool is used in internal projects as a devDependency.

But our repo is also unit tested, and the root path is used fairly similarly to how you’re doing it – to set up fixtures and run hooks in that project folder. I understand that the root path isn’t designed to be used in this manner, but that’s still a good way to run the full integration test with your library.

I guess it’s rather an edge case, but mostly the inconsistency between copy and installScript was more troubling as we’re actually utilising the overwrite flag in our library and would just like to have consistency here.

meister avatar Oct 25 '16 10:10 meister

I guess it’s my own fault – using undocumented feature, and when it’s breaking… And the problem for us was that every time I ran unit tests, it actually wrote the unit test fixtures to the main project’s package.json ;) It’s a good feature though. Some of the utils deserve their own repository, like findProjectRoot/copy.. is also handy in some other internal tools for devs.

meister avatar Oct 25 '16 11:10 meister