Custom logger feature

[gautaz]

Hello @typicode,

I am using husky as part of a scaffolding project and needed to pass husky the logger used from the outer scope.

I do not know if this feature is something you might want to add in husky but here it is. I am quite new to typescript so I might have made some mistakes when specifying the logger interface. This interface defines 3 levels of log, one of which is currently unused (warn), I can remove it if you feel like it will never be necessary or if you want to add it later.

I have also added a test case and extended the documentation a bit. I am open to any advice/modification to make this go upstream if this is something you are ok with.

Gautaz

EDIT:

I have replaced the logger argument by customOptions for the configure method, it seems more relevant and CustomOptions can later be extended with additional features.

I have also removed the warn logger as it was not used (it can be added later anyway if needed).

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[devinrhode2]

Foff stale bot

[gautaz]

@devinrhode2 Thanks for taking care of the stale bot :-).

I took the opportunity to take into account the latest commits on the main branch.

[devinrhode2]

@devinrhode2 Thanks for taking care of the stale bot :-).

I took the opportunity to take into account the latest commits on the main branch.

Is there any good way I can view the diff from before+after your force push?

[gautaz]

@devinrhode2

Is there any good way I can view the diff from before+after your force push?

AFAIK there is no way to do that because a force push basically rewrites the branch history. The reason why I am rewriting the branch history is to avoid multiple merge commits until the branch is merged on the trunk. This avoids loosing the initial commits (the ones that matter in the PR) in a flow of successive merge commits.

It eases the review for a new reviewer but forces persons already reviewing to review the whole PR each time... I guess the choice has to be made depending on the size of the PR. As for this PR, I do not know if you consider it to be a small or a big one 😃.

[gautaz]

Hello @typicode,

I don't mean to be rude but would you please mind having a look at this PR and give me some feedback?

If the current implementation does not match your standards, would you please tell me what would be the right way to do it?

Thanks for your time,

Gautaz

[devinrhode2]

I was cleaning up diff, and not seeing anything alarming, but saw something very interesting:

Good commit history reveals these little bits and pieces making PRs easier to review. After merge, the history is still quite useful for anyone to understand what the actual changes were :)

The bare minimum we need from the tooling ecosystem is for the git diff to at least be able to ignore whitespace/indentation changes!

[gautaz]

Hello @devinrhode2 , thanks for the review :smile:.

Yes, the default github diff does not ignore whitespaces, you can change that easily : https://stackoverflow.com/questions/37007300/how-to-ignore-whitespace-in-github-when-comparing

Applying this renders the following (which shares similarities with your diff but takes into account all my modifications):

As for the this.set, it is necessary to call the set method on the current husky instance produced by the configure function. The drawback is that I had to bind all original exported husky functions as shown at the bottom of my screenshot.

I am open to any suggestion on this matter.

[devinrhode2]

You gave me an idea!

I'm not sure it's truly better, but you could invert things

Instead of housing all the code inside configure, you could turn each method into createInstall, createAdd, etc,

But I think that might produce more code, overall

I think it's good as is!

@typicode note that guataz did change package lock to update all minor versions

Q: I generally like to see accurate versions inside of package.json, but since this is a classic npm/node module... I'm not sure what would be best

My commit history explicitly calls out this package-lock update, which I think would be very good to separate, from what I can tell the minor version bump is completely orthogonal and unrelated, making it a separate commit helps with that.

@guataz I'm just personally curious, what npm command did you run to perform this update?

I wonder if we should only let dependabot update dependencies?

[gautaz]

@devinrhode2 You're right, it's best if the PR does not play with the dependencies versions, it is unrelated to the PR original intent. I will remove this from the PR.

As for the command I used to update the package-lock.json, I may have done something a bit drastic at this time like rm package-lock.json && npm i while handling some conflicts during a rebase (this feels stupid now that I am looking back to it as I should not have modified the lock file at all).

[gautaz]

Done.

The package-lock.json file is still currently modified by npm i for just on line:

Which is because its node requirement is not in sync with the one detailed in package.json. Is that an issue ?

[devinrhode2]

Oh man :(

I had bunch of pending comments that were not submitted only like 1-2 things to tweak tho

[devinrhode2]

Done.

The package-lock.json file is still currently modified by npm i for just on line:

Which is because its node requirement is not in sync with the one detailed in package.json. Is that an issue ?

It's good to get it updated and in sync, that's for sure :)

There could be a PR merge check that runs npm i and see's if package-lock has any changes (and husky being husky, we should run the same check as a pre-commit/pre-push hook)

I do have a pre-commit thing that can ensure package-lock/yarn.lock changes are normalized and predictable.. but it's not really battle tested so shouldn't be advertised to general public

[devinrhode2]

I have 4 "TODO" comments, 2 "optional", 2 seem more meaningful

[gautaz]

I have 4 "TODO" comments, 2 "optional", 2 seem more meaningful

Thanks a lot @devinrhode2 :smile:, I'll look into them ASAP.

[gautaz]

Hello @devinrhode2,

A little recap of the latest modifications:

• the missing backquote in documentation has been fixed
• I followed your recommendation to use Partial for the CustomOptions declaration
• I ensured that there is no shell issue with . "$(dirname "$0")/functions.sh"

Your latest comment mentioned a fourth TODO but I didn't found it... :disappointed:

[devinrhode2]

Having written this yourself @gautaz, what do you think about the warn function again?

I was thinking that if someone wants to pass in a warn function, it shouldn't cause a type error?

I think I am bike shedding at this point, pr can be merged once guataz is ready

[gautaz]

Having written this yourself @gautaz, what do you think about the warn function again? I was thinking that if someone wants to pass in a warn function, it shouldn't cause a type error? I think I am bike shedding at this point, pr can be merged once guataz is ready

@devinrhode2 I just have a doubt about your intent, do you mean that we should forbid someone to pass functions that would remain unused or do you mean that we should add a warn function again? (I am not an english native so I might misunderstood things from time to time)

[gautaz]

Hello again @devinrhode2, I'm not sure you did see my latest comment regarding the warn function. If it's not such a big deal, I guess the PR is ready for merging unless I have forgotten anything else to fix :sweat_smile:.

[devinrhode2]

Sorry dropped the ball

What I was trying to say, is that maybe we add this to the type interface: warn?: (...args: any[]) => void

[gautaz]

What I was trying to say, is that maybe we add this to the type interface: warn?: (...args: any[]) => void

I could have added this in my latest commit but it felt a bit too much as the function is not used for now. Its ready for merging :-).

[devinrhode2]

Agreed

@typicode this is ready to merge imo

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[devinrhode2]

nooooo stalebot!

[gautaz]

nooooo stalebot!

Thanks @devinrhode2 :thumbsup:!

@typicode, enjoy your vacations!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[devinrhode2]

@typicode can you merge? On June 20th I said it's ready to merge (imo)

[gautaz]

Hello @typicode, I can resolve conflicts once again but I cannot do this forever without knowing if this will ever be merged.

Can you please at least state your position regarding this PR?

[typicode]

Truly sorry and thank you for the PR, I will review it for next major release (planned for Node EOL). I'll take care of the potential conflicts if it's merged, you can leave it as it is.

[typicode]

Thanks again for the PR, after long consideration I'd rather not make husky's code more complex, sorry. Especially since it can be achieved without any change in the API.

If really needed (though I think husky shouldn't be silent since it's making changes to user's Git config), it's possible this way with the current version:

const husky = require('husky')

console.log = () => {}
try { husky.install() } catch {}