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

utils.configureHook breaking change in 2.2.1 #61

Open
pipedrive-bot opened this issue Oct 6, 2016 · 3 comments
Open

utils.configureHook breaking change in 2.2.1 #61

pipedrive-bot opened this issue Oct 6, 2016 · 3 comments
Labels

Comments

@pipedrive-bot
Copy link

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.

@nlf
Copy link
Owner

nlf commented Oct 6, 2016

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 nlf added the question label Oct 7, 2016
@meister
Copy link
Contributor

meister commented Oct 25, 2016

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
Copy link
Contributor

meister commented Oct 25, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants