[WIP] Fix #77

[kai-tub]

Add --required option to the filter subcommand. Also add smudge = cat to ensure that dirty notebooks aren't deleted with required.

• Now if required = True and the command fails, reverting (checking out) a modified file will delete it!
  • Test by modifying a notebook from the repository
  • git checkout -q -- <path-to-nb>
  • Will return: fatal: <nb>: smudge filter nb-clean failed and delete the notebook
• The common workaround is to include a smudge simply with cat, which I've done
  • I am not 100% sure what the possible side effects are

I have not written any tests, because I would like to discuss them with you first. I don't really understand what the benefit of mock_git.assert_called_once_with is. For my proposed code I would have to change the mock assertion. This lets me believe that I am possibly adding too much into the git_add function. If the mock should implicitly ensure that the function is only doing one thing, then maybe it could be better to split this one function into multiple smaller ones? What is your opinion on this? I must say that I haven't really used mock before, so I am happy to learn the motivation behind counting the number of calls :)

[srstevenson]

Thanks for working on this, it's looking good!

The call to mock_git.assert_called_once_with ensures we're calling Git with the correct arguments. There isn't too much behind the use of assert_called_once_with instead of assert_called_with: assert_called_with checks that the mock was called at least once, and that the last call had the specified arguments, whereas assert_called_once_with additionally asserts that there was only a single call. This has marginal benefit here, but would enable us catch duplicate calls, or previous calls with incorrect arguments.

As we're changing the behaviour we're testing, it's fine to change the assertions we're making on the mock too. Now we call Git three times (to add the clean filter, smudge filter, and required flag) you could change the assertion to:

from unittest.mock import call

mock_git.assert_has_calls(
    [
        call("config", "filter.nb-clean.clean", filter_command),
        call("config", "filter.nb-clean.required", required),
        call("config", "filter.nb-clean.smudge", "cat"),
    ]
)

[srstevenson]

I'll close this as stale, but please feel free to reopen it in future if you pick this up again. 😃