Add ability to use as a pre-commit hook

[Pacu2]

This should allow using darker as a pre-commit hook in pre-commit.

[akaihola]

Thanks @Pacu2 for this addition! Could you explain this in the README and link to the relevant projects and their documentation for how to make use of this feature?

[akaihola]

Also please add your information to the CONTRIBUTORS.rst file!

[akaihola]

This new feature deserves a mention in the change log (CHANGES.rst). Could you add that as well?

[akaihola]

@Carreau @Mystic-Mirage @CorreyL are you familiar enough with pre-commit to tell whether this patch is good to be merged?

[CorreyL]

@CorreyL are you familiar enough with pre-commit to tell whether this patch is good to be merged?

My old team is actually looking to use darker as a pre-commit hook, so this PR would be useful. I'll see if I have time to look into this tomorrow and try it out for myself and report back, but if @Carreau or @Mystic-Mirage have any personal experience with pre-commit hooks, I will defer to them.

[CorreyL]

I could be mistaken about my conclusion, so someone please step-in and correct me if I'm wrong.

The idea behind a pre-commit hook in git is to run a set of commands prior to the commit message being written.

I tried to run darker as a pre-commit hook, using the pre-commit python package in one of my python projects, setting my .pre-commit-config.yaml as follows:

# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
-   repo: https://github.com/Pacu2/darker
    rev: master
    hooks:
    -   id: darker
files: \.py$

I intentionally added a change that I expect darker and black to re-format:

(venv) λ git add -p
diff --git a/inputs.py b/inputs.py
index 1e4e51f..2654cbe 100644
--- a/inputs.py
+++ b/inputs.py
@@ -5,7 +5,7 @@ def get_year_input(rates):
     tries = 0
     while tries < max_tries:
         print(
-            "What year would you like to get your currency rate from?\n"
+            'What year would you like to get your currency rate from?\n'
             f"Supported years include: {rates.get_supported_years()}"   
         )
         year_input = input()

With the change staged, here is what happens with darker set as a pre-commit hook:

C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git add -p inputs.py
diff --git a/inputs.py b/inputs.py
index 1e4e51f..2654cbe 100644
--- a/inputs.py
+++ b/inputs.py
@@ -5,7 +5,7 @@ def get_year_input(rates):
     tries = 0
     while tries < max_tries:
         print(
-            "What year would you like to get your currency rate from?\n"
+            'What year would you like to get your currency rate from?\n'
             f"Supported years include: {rates.get_supported_years()}"
         )
         year_input = input()
(1/1) Stage this hunk [y,n,q,a,d,e,?]? y


C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git commit
darker...................................................................Passed

Notice how darker marks the commit as Passed, even though I expected the changes to have been reformatted.

This makes sense in the bigger context of what darker is trying to achieve, which is only calling black on un-staged changes getting re-formatted.

As a point of comparison, here is black as a pre-commit hook:

# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
-   repo: https://github.com/psf/black
    rev: 19.3b0
    hooks:
    -   id: black

Here is what happened:

C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git add -p
diff --git a/inputs.py b/inputs.py
index 1e4e51f..2654cbe 100644
--- a/inputs.py
+++ b/inputs.py
@@ -5,7 +5,7 @@ def get_year_input(rates):
     tries = 0
     while tries < max_tries:
         print(
-            "What year would you like to get your currency rate from?\n"
+            'What year would you like to get your currency rate from?\n'
             f"Supported years include: {rates.get_supported_years()}"
         )
         year_input = input()
(1/1) Stage this hunk [y,n,q,a,d,e,?]? y


C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git commit
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted inputs.py
All done! \u2728 \U0001f370 \u2728
1 file reformatted.

Notice how black failed my commit, and it stopped me from writing a commit message, which is what I expected to happen.

As a second experiment, here is another git add -p run, wherein I only accept staging a single chunk:

C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git add -p inputs.py
diff --git a/inputs.py b/inputs.py
index 1e4e51f..374deb6 100644
--- a/inputs.py
+++ b/inputs.py
@@ -5,7 +5,7 @@ def get_year_input(rates):
     tries = 0
     while tries < max_tries:
         print(
-            "What year would you like to get your currency rate from?\n"
+            'What year would you like to get your currency rate from?\n'
             f"Supported years include: {rates.get_supported_years()}"
         )
         year_input = input()
(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? y
@@ -15,7 +15,7 @@ def get_year_input(rates):
         except Exception as err:
             tries = tries + 1
             print(f"{err}\n")
-    raise Exception("Number of tries exceeded. Exitting program.")
+    raise Exception('Number of tries exceeded. Exitting program.')


 def get_month_input(rates):
(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? n
C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git commit
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to C:\Users\Correy\.cache\pre-commit\patch1593632499.
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted inputs.py
All done! \u2728 \U0001f370 \u2728
1 file reformatted.

[INFO] Restored changes from C:\Users\Correy\.cache\pre-commit\patch1593632499.

This is what the reformat did:

Notice how it only modified the chunk I staged for a commit, and failed my attempt to commit.

Overall, I believe we would want darker to run as a pre-stage (i.e. reformat newly added lines before git add -p) hook, but I'm not sure if such functionality is available, based on the git hooks listed here:

https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks#_committing_workflow_hooks

But I definitely stand to be corrected, perhaps there's a way to configure the Python pre-commit package to get my desired behavior? Or perhaps I am using the pre-commit python package incorrectly?

[akaihola]

@CorreyL, if darker had a switch for comparing staging/index to HEAD (instead of worktree to HEAD), would that solve your issue and allow the pre-commit hook to work as expected?

[CorreyL]

if darker had a switch for comparing staging/index) to HEAD (instead of worktree to HEAD), would that solve your issue and allow the pre-commit hook to work as expected?

I believe this would work. To ensure we're on the same page, if this feature were added, the workflow I envision would look something like:

C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git add -p
diff --git a/inputs.py b/inputs.py
index 1e4e51f..2654cbe 100644
--- a/inputs.py
+++ b/inputs.py
@@ -5,7 +5,7 @@ def get_year_input(rates):
     tries = 0
     while tries < max_tries:
         print(
-            "What year would you like to get your currency rate from?\n"
+            'What year would you like to get your currency rate from?\n'
             f"Supported years include: {rates.get_supported_years()}"
         )
         year_input = input()
(1/1) Stage this hunk [y,n,q,a,d,e,?]? y


C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git commit
darker....................................................................Failed
- hook id: darker
- files were modified by this hook

reformatted inputs.py
All done! \u2728 \U0001f370 \u2728
1 file reformatted.

# darker would have reverted the changes made to `inputs.py`

Though I do foresee a couple of potential gotchas still:

1. I'm not sure how to support command line arguments in the pre-commit python package
2. The originally staged changes from the above git add -p execution would still be staged, i.e.

# Carrying on from my above example

C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
(venv) λ git commit
darker....................................................................Failed
- hook id: darker
- files were modified by this hook

reformatted inputs.py
All done! \u2728 \U0001f370 \u2728
1 file reformatted.

# darker would have reverted the changes made to `inputs.py`

C:\Users\Correy\projects\personal\sassa-git-workshop (feature/pre-commit-hook) 
λ git diff --staged
diff --git a/inputs.py b/inputs.py
index 1e4e51f..2654cbe 100644
--- a/inputs.py
+++ b/inputs.py
@@ -5,7 +5,7 @@ def get_year_input(rates):
     tries = 0
     while tries < max_tries:
         print(
-            "What year would you like to get your currency rate from?\n"
+            'What year would you like to get your currency rate from?\n'
             f"Supported years include: {rates.get_supported_years()}"
         )
         year_input = input()

This is what happened when I used black as a pre-commit hook; it would not automatically stage the formatted changes.

I'm not sure if darker could perhaps automatically re-stage the formatted changes? Admittedly, that might be out of scope for darker itself though.

[akaihola]

I'm not sure if darker could perhaps automatically re-stage the formatted changes? Admittedly, that might be out of scope for darker itself though.

Well, darker does already interact with Git (albeit only for obtaining information currently)

Currently it

• compares the working tree (files on disk) to HEAD, and
• modifies files in the working tree (which of course is only a filesystem operation, not involving Git).

With e.g. a --staged or --cached option (named after the ones in git diff), it could instead

• compare the staging index to HEAD, and, symmetrically,
• modify files in the staging index.

For me it doesn't sound like pushing darker's responsibilities too far yet.

I'm not sure how to support command line arguments in the pre-commit python package

Do you mean providing the --staged or --cached option for darker? If arguments aren't supported in pre-commit, we could add a separate console entry point for a pre-commit mode.

By the way, darker also currently requires at least one file or directory path argument on the command line. Perhaps it could default to the current directory?

[akaihola]

Uh-oh @CorreyL, I think I get your point about staging only some chunks in a file.

If darker only reformats a chunk in the staging index, the unformatted version of that chunk in the working tree will later look like an intended change from the developer to revert formatting.

Having darker take the diff from reformatting the staging index and apply it also to the working tree should work in most cases, but I can imagine complex situations where it would simply fail or do the wrong thing. And, the changes in the staging index don't necessarily even correspond to the working tree – staging might have been followed by a working tree edit which falls inside an already staged chunk.

Would it then actually be most appropriate to run darker twice: once for the working tree and once for the staging index? Would that ensure that the two are as much in sync as possible after the pre-commit hook?

[akaihola]

For editing files in the staging index, I found these resources:

• Git modify contents of index directly for pre-commit formatting hook) on Stack Overflow
• Keyword Expansion and "smudge" and "clean" filters in the Pro Git book

But is it really so that pre-commit doesn't expose the index on the filesystem, and requires the pre-commit hook scripts to access the index through Git internals?!?

Or is it just that pre-commit hooks aren't supposed to modify files, but just report violations?

[akaihola]

We rebased this PR on top of current master (see #91). It seems the default behavior is now what pre-commit expects – compare files on disk (whether in staging index or not) to HEAD, modify files in-place and exit with a zero return value.

@samoylovfp and Sean (whatever your GitHub handle is), please review #91.

[Pacu2]

@akaihola thanks for picking this up. Looking forward to using darker as a precommit in my project. Any help needed let me know.

[akaihola]

Closed in favor of #91.

[akaihola]

Long after this was discussed implemented, Darker now fails to work for pre-commit when both from-ref and to-ref are specified. See #180. I have difficulty understanding what Darker should actually do there. Maybe you @Pacu2 or @CorreyL can figure it out?