Adding git pre commit hook for formatting staged changes

[deyaaeldeen]

This hook will run on every commit to run prettier on the staged changes only (not whole files). The prettier configuration file used is https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/.prettierrc.json. More details about the hook can be found here.

To install the hook, run rush update.

[mikeharder]

I have no objections to adding this tool if the language team supports it (and it appears they do). However, I feel like there should be a better way to enable it. Could it be enabled in package.json, which I believe is listed as the recommended way here?

https://www.npmjs.com/package/precise-commits#2-precommit-hook

[deyaaeldeen]

@mikeharder Thanks for your feedback. However, I am trying to avoid spamming all package.json files with the same script that is not intended to be executed manually anyway.

[mikeharder]

@mikeharder Thanks for your feedback. However, I am trying to avoid spamming all package.json files with the same script that is not intended to be executed manually anyway.

I don't think adding something to every package.json file is a bad thing. Our package.json files already have many of the same dev dependencies and script commands, and rush makes it easy to keep the versions aligned across packages and updated regularly.

Other than needing to change every package.json file, what are the pros and cons of enabling this tool via package.json as opposed to the rush pre-commit hooks? Would it be valuable to give devs the ability to run this tool manually via npm run precise-commits, say if they wanted to review the prettier changes before committing?

We should use the option we think gives the best overall developer experience, ignoring whether we need to modify every package.json or not.

[mikeharder]

If we decide there is no value in adding this command to every package.json, then I agree this PR aligns with the guidance at https://rushjs.io/pages/maintainer/git_hooks/.

[deyaaeldeen]

what are the pros and cons of enabling this tool via package.json as opposed to the rush pre-commit hooks?

@mikeharder Using rush to manage the hook ensures that the hook will always be installed in the right place .git/hooks/pre-commit once we run rush update, a common operation in our workflow. On the other side, to have the same behavior without rush's help, we will need an extra package (husky) to install the hook using npm. Otherwise, expressiveness of the hook scripts are the same for both approaches.

Would it be valuable to give devs the ability to run this tool manually via npm run precise-commits, say if they wanted to review the prettier changes before committing?

I do not think so. In fact, the hook will not commit the formatting changes and will leave it up to you to decide if you want to commit them. With this workflow, I can not think of a scenario where you would want to run the formatter manually. Ideally, prettier is already running in your editor, so this is more of a soft alarm against accidental bad formatting. Eventually, I would like to add to our CI a formatting check similar to the one described here.

We should use the option we think gives the best overall developer experience, ignoring whether we need to modify every package.json or not.

Duplication generally just does not sit right with me but this is a good and important point.

[mikeharder]

Adding this feature via the rush mechanisms is simpler than modifying every package.json, so I am fine merging this PR as-is, and we can convert to the package.json method in the future if we decide it's better.

[ellismg]

Just wondering - should we actually exit with a non zero status here to block the commit or are we okay with allowing the commit to happen even if we were unable to run the prettier on it?

[deyaaeldeen]

I do not see why we might want to block the commit, do you have a reason in mind? Ideally, this case should never happen if rush is doing its job. However if for whatever reason rush state was corrupted and the binaries are missing, I doubt that aborting the commit will accomplish anything for the developer experience.

[ellismg]

I guess it's about how strong we are trying to push the enforcement of this behavior. Blocking the commit would make it harder to end up checking in code in cases where your environment is horked, but maybe we think that's a rare enough occurrence and that even if the code gets checked in without having been run through prettier it's not the end of the world.

Happy with whatever call you make, was just trying to get a sense for how strict we were trying to be here.

[deyaaeldeen]

Yeah, sorry I did not make a good write-up about my approach earlier. I wrote the assumptions I work with in a comment below.

[chradek]

Looks great, thanks for adding this @deyaaeldeen!

[willmtemple]

This is a great idea and there is prior art for doing this in the Rush Stack repo itself.

I think it's a little awkward that the hook fixes the files (but doesn't stage the fixes) and then allows the commit to go ahead anyway. Maybe we could either silently fix the formatting errors and stage the fixes before the commit, or do check only and reject the commit if the check fails.

[willmtemple]

I think we should consider capturing the exit status here and passing it through at the end.

[deyaaeldeen]

This could cause the commit to fail, which conflicts with assumption 4 in my comment below. I am happy to change this if we get a consensus on another approach with another set of assumptions.

[deyaaeldeen]

After today's offline discussion, we will wait to get acknowledgement from our partners. @ramya-rao-a would you please ask them to put a review/acknowledgement?

For the sake of completeness, I will outline below the assumptions I made and how this hook affects our workflow.

Assumptions:
1- It is very likely that prettier already runs in your editor using the configuration files located in the sdk directory.
2- You do not mind waiting anywhere between well less than a second and a few seconds when you issue a git commit to run the hook script first before your commit is actually performed. This waiting time depends for the most part on how many files have been changed. So for the typical case of a few files, the waiting time is less than a second.
3- You could be interested in looking at the formatting changes before actually commit them.
4- You want to move fast so you do not want to get your commit aborted for any reason.
5- It does not matter how many commits you might need to make because they will always get squashed at the time of merging with master.
6- You think of the git hook as an aid not as an enforcement facility.

Description:
This is a pre commit git hook that runs when you issue a git commit. The script calls precise-commits which in turn calls prettier on the changed parts of the changed files and will write the result to disk, if any, without staging or committing them. Consequently, the commit operation will go through and all your original staged changed will only be part of the commit. After the commit, if there are formatting changes, they will remain unstaged so you can inspect, stage, and commit them.

Of course, not everyone will like the assumptions I made and another hook behavior might suit them better. So to get an idea about the consensus, please upvote this comment if this works for you. If not, please post another comment with your assumptions and a description of the behavior you like more.

[ramya-rao-a]

cc @southpolesteve, @zfoster, @XiaoningLiu

[XiaoningLiu]

That's a great step! Please make sure win/linux/mac can work with this. Looks good.

[deyaaeldeen]

That's a great step! Please make sure win/linux/mac can work with this. Looks good.

I verified that it works on linux (including wsl) and in git bash on windows.

[deyaaeldeen]

I just noticed that the precise-commits repo is stall for two years now. I reached out to the repo owners asking them what is their plan for this project. In the mean time, I propose to keep using it as long as it works which is compatible with assumptions 1 and 6 here.

[deyaaeldeen]

prettier has been run across our packages (see #10269, #10273, #10272, #10271, #10270, #10268, #10267, #10266, #10265, #10264, #10263, #10262, #10261), so this hook should work on new changes only, without introducing any noise. Furthermore, merging this PR clears the path to merge #10145. Now that everyone is onboard with this, I will go ahead and merge it.