Enable integration with pre-commit #698

JohnPaton · 2020-12-30T18:06:02Z

This is a first attempt at an implementation of the suggestion in #691 to enable "proper" integration with the pre-commit framework. The changes summarized:

A new .pre-commit-hooks.yaml file, which tells pre-commit how to install this package (pre-commit does this internally), which command to run, and what the default options are
A new cli argument, --add-untracked, which adds any output files that weren't previously tracked by git to the index. Two points to note here:
- The reason we are tracking only new files is that pre-commit doesn't consider changes to them if they aren't in the index (pre-commit requires all files in the index be unchanged by all hooks before passing)
- The command will return a non-zero status code in case any newly generated files are actually added. This is because by adding them "on the fly" during the hook, pre-commit doesn't pass them to any other configured hooks for further checks, so they could be wrongly accepted into the commit.

The net effect is a proper pre-commit hook, which will only allow a commit as long as the configured Jupytext command doesn't change any of the files in the index, or add any new ones. This is how pre-commit's hooks are expected to operate.

Questions:

This is a bit of an adjustment to how Jupytext's main command is operating - is it too much? How easy would it be to make this separate/not expose the --add-untracked option so publicly?
How to test this behaviour (will be investigating, this is a WIP)?

Skylion007 · 2021-01-06T18:59:39Z

I was actually just about to make this PR myself that I mentioned back in #580, @johnpatton :). The untracked feature is nice, but I think its antipattern to have any pre-commit hooks modify the git index, particular because of how pre-commit infra does a git stash / git pop at the beginning/end of dirty repos. I worry this could cause errors and corrupt the repo (it should definitely complain about it, but the user should be forced to update the git index manually after the pre-commit hook as has run.

Skylion007 · 2021-01-06T19:02:20Z

The better way of doing this to default the --sync behavior imo and as that will properly modify any properly configured NB and associated scripts and do nothing else. Users can do more sophisticated behavior by manually specifying additional args.

.pre-commit-hooks.yaml

jupytext/cli.py

Skylion007

Great job on the test suite, but ultimately I think this pre-commit hook is too narrow/specific and doesn't address other common use cases of JuPyText (like generating Google Colab notebooks from scripts).

@JohnPaton Happy to make the changes myself on this PR if given write access.

docs/using-pre-commit.md

Skylion007 · 2021-01-06T19:07:12Z

docs/using-pre-commit.md

+    hooks:
+    - id: jupytext
+      args: [--to, py:percent, --pipe, black]
+-   repo: https://github.com/psf/black


This isn't necessary or dieal and could conflict with otehr black hooks. It should just be added as an additional_dependency but the end user if needed at all.

It's just an informational example, it's very normal to have a black hook already present for your python code and if you want your generated code not to get into a hook fail loop, you have to pipe it to black

Oh this is abusing the virtualenv of the pre-commit hook (it should only require the bear minimum requirements and other hooks if they absolutely build upon one another). It should really be installed using additional_depenencies if needed by the end user (see the blog post test I adjusted.

Added black in additional_depenencies

Yes I agree with additional_dependencies (I think there is a typo in additional_depenencies).

Also you say you must ensure that all hooks will pass (...).
I understand that we are obliged to do so when we use --to, but maybe if we use --sync it's just "more convenient" as we need to run the hook just once, not twice. Is that correct?

@JohnPaton , what would you think of removing this example? I was trying to add a test for it, but we seem to have tricky issues - maybe we add that in another PR?

@requires_pre_commit def test_pre_commit_hook_sync_black(tmpdir): # get the path and revision of this repo, to use with pre-commit repo_root = str(Path(__file__).parent.parent.resolve()) repo_rev = system("git", "rev-parse", "HEAD", cwd=repo_root).strip() git = git_in_tmpdir(tmpdir) # set up the tmpdir repo with pre-commit pre_commit_config_yaml = dedent( f""" repos: - repo: {repo_root} rev: {repo_rev} hooks: - id: jupytext args: [--sync, --pipe, black] additional_dependencies: - black==19.10b0 # Matches hook - repo: https://github.com/psf/black rev: 19.10b0 hooks: - id: black language_version: python3 """ ) tmpdir.join(".pre-commit-config.yaml").write(pre_commit_config_yaml) git("add", ".pre-commit-config.yaml") with tmpdir.as_cwd(): pre_commit(["install", "--install-hooks"]) # write test notebook and output file nb = new_notebook( cells=[new_code_cell("1+1")], metadata={ "jupytext": {"formats": "ipynb,py:percent", "main_language": "python"} }, ) nb_file = tmpdir.join("test.ipynb") py_file = tmpdir.join("test.py") write(nb, str(nb_file)) git("add", ".") # First attempt to commit fails with message # "Output file test.py is not tracked in the git index" with pytest.raises(SystemExit): git("commit", "-m", "fails") git("commit", "-m", "succeeds") assert "test.ipynb" in git("ls-files") assert "test.py" in git("ls-files") # py_file should have been reformated by black assert "\n1 + 1\n" in py_file.read_text()

Skylion007 · 2021-01-06T19:09:39Z

docs/using-pre-commit.md

@@ -27,35 +27,41 @@ Note that these hooks do not update the `.ipynb` notebook when you pull. Make su

 ## Using Jupytext with the pre-commit package manager

-Using Jupytext with the [pre-commit package manager](https://pre-commit.com/) is another option. You could add the following to your `.pre-commit-config.yaml` file:
-```
+Using Jupytext with the [pre-commit package manager](https://pre-commit.com/) is another option. You could add the following to your `.pre-commit-config.yaml` file to convert all staged notebooks to python scripts in `py:percent` format (the default):


There is a use case where the old way of doing it actually preferred, and that's when you want to run locally installed code. This commit hook is great for a general use case where conversion is the only part of JuPyText you want to use, but execution requires being in the local sandbox.

I'm not sure I understand this comment. Why does it matter if it's installed locally or in the sandbox for this case?

Requirements that can't be specified by pip install or are difficult to (like say the local master version of a pip package).

Skylion007 · 2021-01-06T19:10:01Z

docs/using-pre-commit.md

 repos:
-   repo: local
+-   repo: https://github.com/mwouts/jupytext
+    rev: master


This really should be pinned to a specific version or left for the user to specify. Otherwise it can cause a bunch of issues.

Again, this is an example.

Skylion007 · 2021-01-06T19:21:30Z

.pre-commit-hooks.yaml

+  language: python
+  entry: jupytext --add-untracked
+  pass_filenames: true
+  files: ipynb


Instead of files why not use types: [jupyter] like nbstripout uses?

It didn't work on my machine, probably I had an older pre-commit installed but I wasn't sure when it was added so I did it this way. But we will remove this and just pass all files, ignoring the ones that we aren't trying to process.

@JohnPaton, yeah I've run into this before. You need to run pip install -U identify. Annoying that you can specify a min version of identify in the hook (you can for pre-commit though), but it is the recommended way to do it and will make it a lot easier for end users to override the behavior and support other types.

Suggested change

files: ipynb

types: [ipynb, md, rst, python]

list of supported types: https://github.com/pre-commit/identify/blob/master/identify/extensions.py

Skylion007 · 2021-01-06T19:34:28Z

docs/using-pre-commit.md

-        language: system
+    - id: jupytext
+      args: [--to, py:light]
+    - id: jupytext


Why not have one hook id with --to py:light --to markdown it's much more efficient.

(I agree with demonstrating multiple hooks, but this is not the use case for them)

Because I didn't know Jupytext supported that syntax, thanks :)

tests/test_pre_commit.py

mwouts · 2021-01-06T22:17:23Z

Hi @Skylion007 , thanks for joining the conversation! Well with @JohnPaton and yourself in the thread I am going to learn a LOT about pre-commit hooks, I am looking forward to it!

I don't know what you will think about that, but... I'd like to make the choices about this hook in a test-driven manner. I mean, I am thinking of a series of use cases that I'd like to see supported, like

all the notebooks in the repo are paired to py:percent files, and the .ipynb files are in .gitignore, so they don't reach the repository (the hook updates the py file from the .ipynb file that the user may be modifiying outside of Jupyter)
the notebooks are paired to py:percent files. The .ipynb files are committed with no outputs, but still the user can keep the outputs locally. And the hook ensures that modifications in whatever file are reported to the other one
the notebooks are paired to py:percent files, and synched with the .ipynb files. If a notebook was modified it is re-executed before the commit (I'm not so sure that this case makes sense - or do we want to reexecute the notebook only if the text version was changed?)

In addition to these use cases, I'd like to document and test (IMO the main purpose of writing the blog is to test that this is going to be usable 😄) how to

store the text versions and ipynb files in distinct folders (probably not related to the hook, this will directly depend on Jupytext)
make sure that the py versions of the notebooks meets quality requirements (flake8) or formatting (black) - maybe this is not for our hook, but for a standard hook on py files
and how to reformat the long lines in markdown cells into shorter ones, compliant with pep8

Which are your own expectations? How big is the overlap with mine?

Regarding the timing, I hope to have a draft for the blog by tomorrow or Friday, I'll share it with you both then, and then I hope to be able to iterate quite fast, within one or two weeks I mean.

Skylion007 · 2021-01-07T17:13:37Z

In regards to three use cases:

This is fine as long as you don't modify the git staging. We could add an optional flag to throw errors if one of the edited files are not added to the index, instead of throwing it.. It doesn't matter if a file is in a gitignore if the file is added / currently staged for the commit.
Is actually very similar to what we do in habitat-sim and is accomplished by having two hooks (nbstripout being a seperate one to ensure all committed notebooks have no outputs). It won't edit the output of local files unless you try to commit them and those changes can be easily reverted / fixed with git pop/stash.
Is easy with default pre-commit settings, if you have --sync set, you can also have --execute. pre-commit will only execute on files that staged and changed in commit (unless manually run with --all-files).

By default, pre-commit hooks will only run on edited files for speed. Having it run on all files / the entire configuration is easily doable though.

Yeah not related to the hook. Just the args that can be passed into the hook / the jupytext config.
Easily done in both actually, with pipe + additional_requirements or by just editing the python files. I actually run them on both, but really only the editing the text files is necessary (requires two pre-commit runs in the later case).
This seems unrelated and be can handled with pipe / additional_depenencies IMO.

I'd say the overlap is quite large.

mwouts · 2021-01-08T09:17:11Z

John, Aaron, I've invited you to https://github.com/mwouts/pre_commit_hooks_for_jupyter_notebooks.
It is very much a work in progress, though.

The first thing that I'd like to chat about is this "test" (WIP as well):
https://github.com/mwouts/pre_commit_hooks_for_jupyter_notebooks/blob/ac60bcbcec5ff65c171f8bbb03cc13a795e88a7d/examples/py_percent_sync_with_ipynb_with_outputs/test_py_percent_sync_with_ipynb_with_outputs.py#L35-L64

def test_py_percent_sync_with_ipynb_with_outputs(tmpdir, capsys):
    # Create an empty repository and initialize it
    repo = Repo.init(tmpdir)

    # Add the .pre-commit-config.yaml that corresponds to this example
    create_pre_commit_config_yaml_file(tmpdir, __file__)
    repo.index.add([".pre-commit-config.yaml"])
    repo.index.commit("Add .pre-commit-config.yaml")

    # Install pre-commit
    with working_directory(tmpdir):
        pre_commit(["install"])

    # Add a .jupytext.toml file that pairs all notebooks to py:percent files
    tmpdir.join(".jupytext.toml").write_text(
        "default_jupytext_formats = ipynb,py:percent"
    )
    repo.index.add([".jupytext.toml"])
    repo.index.commit("Add .jupytext.toml")

    # Add an ipynb file
    # Try to commit => pre-commit should create the py file, and fail!
    # Try to commit => pre-commit should complain that the py file is not in the index
    # Add the py file to the index
    # And commit (OK)

    # Modify the py file
    # Try to commit => pre-commit should complain that the ipynb file is not in the index
    # Add the ipynb file
    # And commit (OK)

I like the idea to use such a test to document explicitly our expectations. But at this stage I am not sure that pre_commit(["install"]) in the tmpdir works. Then, do my comments match what we said on the hook? Do you deem this as user friendly (I mean, failing often and requiring 'git add')?

Skylion007 · 2021-01-08T15:22:54Z

.pre-commit-hooks.yaml

@@ -0,0 +1,10 @@
+- id: jupytext


We need a copy of this hook using the system language so that it runs off the system version of JuPytext, happy to write it @JohnPaton . We can even reuse the node by taking advantage of some nice advanced YAML features.

Why do we need that? Users can just do (incomplete example)

- repo: local hooks: id: jupytext-local language: system entry: jupytext

if they want that

.pre-commit-hooks.yaml

Skylion007

Made some suggestions

JohnPaton · 2021-01-08T17:25:42Z

I've given you write access to the PR @Skylion007 - I've got a pretty busy few days ahead so I'll be a bit less active than I'd like, sorry in advance

jupytext/cli.py

JohnPaton · 2021-01-09T12:24:53Z

Okay @mwouts @Skylion007, I've made some pretty big changes based on all the feedback (really feeling this collab guys, nice working with you 🏆)

I removed --add-untracked, it's indeed wrong to modify the index. Instead, I've added two new flags, which will be used together in the hook:

--ignore-unmatched so that pre-commit (or users, if they really want) can pass all the filenames and we can just ignore them if they don't match our expected input format
--alert-untracked which causes Jupytext to exit non-zero if the output files aren't tracked in the git index. That way we can always be sure that if the hook is making changes, then either:
- the files are tracked so pre-commit will fail the hook if they are modified, or
- the files are untracked, and --alert-untracked will fail the hook.
In each case the changes have to be added to the index by the user before the hook can succeed

I have also removed default arguments beyond those two from the hook configuration, so that the user is forced to specify what they are trying to do.

I still need to update the docs but wanted your feedback :)

.pre-commit-hooks.yaml

docs/using-pre-commit.md

mwouts · 2021-01-10T22:48:40Z

Hi @JohnPaton , @Skylion007 , thank you so much for your work here, I also enjoy very much this collaboration!

I completely agree with the --alert-untracked argument, you've convinced me that it is a better idea than the previous --add-untracked.

Regarding the --ignore-unmatched, I am not so sure that it is necessary. I expect that jupytext --sync filename.ext does not do anything if filename.ext is not the name of a paired notebook (well it reads the file and then issues a warning, but no error). It only fails if ext is not a notebook extension (i.e. something else than ipynb, md, py, 'R', etc).

Also, while trying the hook I saw a potential issue with the new build system for Jupytext - @JohnPaton you might not have seen it before because it was introduced just last week. I'll report on that at #706 .

JohnPaton · 2021-01-11T10:30:53Z

I also hit that build issue now @mwouts because I rebased 😬

JohnPaton · 2021-01-11T10:31:54Z

I expect that jupytext --sync filename.ext does not do anything if filename.ext is not the name of a paired notebook

Hmmm, but if they are not using --sync, but --from --to, then it does throw errors

mwouts · 2021-01-11T10:50:26Z

Hmmm, but if they are not using --sync, but --from --to, then it does throw errors

Oh yes that's right... By the way I wanted to ask why you don't add --sync to the defaut command, but I think your comment answers my question, you want to run --to in some cases.

Regarding the build issue, I'll try to address it tonight. Meanwhile a quick fix would be to comment out the part of setup.py that depends on jupyter-packaging - you won't need that part of the package (the extension for Jupyter Lab) in the pre-commit hook anyway.

.pre-commit-hooks.yaml

mwouts · 2021-01-11T21:50:01Z

docs/using-pre-commit.md

+    rev: #CURRENT_TAG/COMMIT_HASH
+    hooks:
+    - id: jupytext
+      args: [--from, ipynb, --to, py:light, --to, markdown]


Oh sorry there can only be one value for --to

Ah my bad for that suggestion.

We still have to update the doc - would you like to take 07b323b?

JohnPaton · 2021-01-18T16:05:07Z

Almost there I think. The one last problem is that the hook seems to be updating both files in --sync mode (I was expecting only the older file to be updated). Because both files are being updated, the hook doesn't know which set of updates to take, so it fails... thoughts?

jupytext.................................................................Failed
- hook id: jupytext
- files were modified by this hook

[jupytext] Ignoring unmatched input path .pre-commit-config.yaml
[jupytext] Reading test.ipynb in format ipynb
[jupytext] Updating 'test.py'
[jupytext] Reading test.py in format py
[jupytext] Loading 'test.ipynb'
[jupytext] Updating 'test.ipynb'
[jupytext] Updating 'test.py'

[WARNING] Stashed changes conflicted with hook auto-fixes... Rolling back fixes...

mwouts · 2021-01-18T23:15:49Z

Thank you @JohnPaton , you've done a hard work here!

Well you're right, when the .py file is more recent than the .ipynb file, --sync updates the .ipynb file and then updates (the timestamp) of the .py file.

Now I'll try to explain the reason for that... it is rooted in the other usage of Jupytext - when Jupytext is used within Jupyter. In that context, the user may edit either the .py file or the .ipynb file. We always take the .py file as the source for inputs, and we throw an error (we ask the user to delete one of the two files) if the .py file is older than the .ipynb file. This works (and is compatible with --sync, that was developed later on) as Jupytext always writes the .ipynb file before the .py version.

Maybe my preferred way to solve this would be, thus, to find a way to update the timestamp of the .py file (currently done at https://github.com/mwouts/jupytext/blob/master/jupytext/cli.py#L691-L693 by actually writing the file) that does not attract the attention of pre-commit. Do you think this is an option?

Another option is to assume that users of the pre-commit hook won't be using Jupytext's contents manager, and to skip the update of the unmodified file.

Or maybe the problem will exist anyway (.ipynb more recent than .py especially on large files) when we pull a commit from a remote? If so, maybe we could skip the update of the unmodified file, and use a post-commit hook to finally update the (timestamp) of the .py file to make it more recent than the .ipynb?

@Skylion007 , did that an issue appear in the local version of the hook? Do you have any advice on how to address this? Thanks

Skylion007 · 2021-01-19T02:28:43Z

Yeah, to be honest the way I solve this is by deleting one of the files and then rerunning it. It's not elegant, but it works. Yeah this is one of the biggest issues with using it as a pre-commit hook currently. Updating the timestamps of both files might be the way to go here. We can safely do that now that we require_serial: True in our JuPyText hook. We don't need to actually write the file btw, we can set the date modified by using os.utime https://docs.python.org/3/library/os.html#os.utime I prefer this solution as it still satisfies the JuPyText contents manager and frankly both files should be updated when one is modified (even if the update is a NOOP). This behavior could be changed by a flag (we may want to group all these behaviors under a master flag pre-commit mode).

…

On Mon, Jan 18, 2021 at 6:16 PM Marc Wouts ***@***.***> wrote: Thank you @JohnPaton <https://github.com/JohnPaton> , you've done a hard work here! Well you're right, when the .py file is more recent than the .ipynb file, --sync updates the .ipynb file and then updates (the timestamp) of the .py file. Now I'll try to explain the reason for that... it is rooted in the other usage of Jupytext - when Jupytext is used within Jupyter. In that context, the user may edit either the .py file or the .ipynb file. We always take the .py file as the source for inputs, and we *throw an error* (we ask the user to delete one of the two files) if the .py file is older than the .ipynb file. This works (and is compatible with --sync, that was developed later on) as Jupytext always writes the .ipynb file before the .py version. Maybe my preferred way to solve this would be, thus, to find a way to update the timestamp of the .py file (currently done at https://github.com/mwouts/jupytext/blob/master/jupytext/cli.py#L691-L693 by actually writing the file) that does not attract the attention of pre-commit. Do you think this is an option? Another option is to assume that users of the pre-commit hook won't be using Jupytext's contents manager, and to skip the update of the unmodified file. Or maybe the problem will exist anyway (.ipynb more recent than .py especially on large files) when we pull a commit from a remote? If so, maybe we could skip the update of the unmodified file, and use a post-commit hook to finally update the (timestamp) of the .py file to make it more recent than the .ipynb? @Skylion007 <https://github.com/Skylion007> , did that an issue appear in the local version of the hook? Do you have any advice on how to address this? Thanks — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#698 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPVMX2PHQO3KGFFZ765P4TS2S6LFANCNFSM4VOSIZLQ> .

mwouts · 2021-01-19T10:01:00Z

We don't need to actually write the file btw, we can set the date modified by using os.utime

Yes! I'll try to do that tonight (don't rewrite the .py file but just update the last modification time). I think it should be the default mode in --sync - no need for something specific to the pre-commit mode here, hopefully. But... will that work with pre-commit? How does pre-commit detect that the file has changed?

JohnPaton · 2021-01-19T15:00:27Z

If it was only timestamps, it would notify that the timestamp is being updated, right? I don't see any

"[jupytext] Sync timestamp of '{}'".format(nb_file)

line in my output above

Skylion007 · 2021-01-19T17:57:17Z

I haven't checked, but I believe pre-commit uses git to see which files don't match the index. It looks like git doesn't store any metadata about the file like ACLs or modified time so that might work: https://stackoverflow.com/a/2179825 https://github.com/pre-commit/pre-commit/blob/bf85379619e4aa5019412f3f3237a8c3c3e225e3/pre_commit/commands/run.py#L199

mwouts · 2021-01-19T18:16:59Z

Hi @JohnPaton , maybe this commit 22d6961 can help? It generalizes the timestamp update (already there for the --to mode) to the --sync mode.

JohnPaton · 2021-01-20T09:18:16Z

I'm afraid that wasn't enough either, it seems like both files are still being modified

jupytext.................................................................Failed
- hook id: jupytext
- files were modified by this hook

[jupytext] Ignoring unmatched input path .pre-commit-config.yaml
[jupytext] Reading test.ipynb in format ipynb
[jupytext] Updating 'test.py'
[jupytext] Reading test.py in format py
[jupytext] Loading 'test.ipynb'
[jupytext] Updating 'test.ipynb'
[jupytext] Sync timestamp of 'test.py'

[WARNING] Stashed changes conflicted with hook auto-fixes... Rolling back fixes...

Not sure if this matters: pre-commit passes the files individually, iiuc, so it will call jupytext --sync test.ipynb and then jupytext --sync test.py

mwouts · 2021-01-20T14:46:54Z

Hi @JohnPaton , yes you're right the issue is with the fact that jupytext --sync is called twice. On the second call the .py file is more recent so the .ipynb file is updated.

Here are a few workarounds that I am thinking of...

run the hook only on .ipynb files
call jupytext just once on the set of all files (and change jupytext/cli.py to run jupytext exactly once per paired notebook)
update jupytext/cli.py again, and skip the calls to 'write' if we see that the updated version matches the current file

I think that 1. is clearly the simple approach. And FYI I might also consider 3 because I think it will be required in case of triple-paired notebooks (.ipynb + .md + .py) (but be reassured, we will keep that for later on)

JohnPaton · 2021-01-20T15:33:02Z

Alright, let's go with option 1 for now then! Then we can get this bad boy merged 🚀

Relevant tests will be skipped in that case

mwouts · 2021-01-20T19:09:30Z

Hi @JohnPaton , @Skylion007 , we're getting ready!

I am done with the review and I'd like to discuss a few details:

The example with --to in the doc needs to be updated to match the one actually tested (use e.g. 07b323b)
Maybe we should temporarily remove the --pipe example from the documentation - at least I can't get it to work in the tests for now. I think option 3 from my previous comment could help here, but I'd prefer to re-add the example later on when it is ready.
How do you want to see this merged? Should I squash or rebase? If we squash, are you both OK for giving the full authorship to John (and I'll cite you both in the changelog)? Sorry for asking, it is the first time I see a PR with so many commits and contributors 😄
Github says that there is a conflict with the base branch, is it easy for you to solve? (Well if we decide to squash, don't bother with that, as I should be able to do it locally)

Skylion007 · 2021-01-20T20:19:55Z

@mwouts Squashing commits will preserve co-authorship if done correctly through the Github API (via the co-author tag in the commit description). Just leaving the default verbose commit description should preserve everything.

update jupytext/cli.py again, and skip the calls to 'write' if we see that the updated version matches the current file

I'd much prefer 3 in the long term. It's sane behavior.

mwouts · 2021-01-21T05:42:26Z

Done! Thank you so much @JohnPaton and @Skylion007 . I am so happy to see this merged! What an impressive work!

I will create a follow-up issue to keep track of the few improvements that we have identified above, and also try to make progress on the blog post... I'll keep you posted!

JohnPaton force-pushed the pre-commit-hooks branch 2 times, most recently from a0b7303 to b5330b2 Compare January 5, 2021 16:57

Skylion007 reviewed Jan 6, 2021

View reviewed changes

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved

Skylion007 reviewed Jan 6, 2021

View reviewed changes

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved

Skylion007 reviewed Jan 6, 2021

View reviewed changes

jupytext/cli.py Outdated Show resolved Hide resolved

Skylion007 suggested changes Jan 6, 2021

View reviewed changes

Skylion007 reviewed Jan 6, 2021

View reviewed changes

tests/test_pre_commit.py Outdated Show resolved Hide resolved

Skylion007 reviewed Jan 8, 2021

View reviewed changes

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved

Skylion007 suggested changes Jan 8, 2021

View reviewed changes

JohnPaton commented Jan 9, 2021

View reviewed changes

jupytext/cli.py Outdated Show resolved Hide resolved

JohnPaton force-pushed the pre-commit-hooks branch from db9c36a to a25df97 Compare January 9, 2021 12:20

Skylion007 reviewed Jan 9, 2021

View reviewed changes

.pre-commit-hooks.yaml Show resolved Hide resolved

Skylion007 reviewed Jan 9, 2021

View reviewed changes

docs/using-pre-commit.md Outdated Show resolved Hide resolved

mwouts mentioned this pull request Jan 10, 2021

"pip install ." for Jupytext requires nodejs #706

Closed

Skylion007 reviewed Jan 11, 2021

View reviewed changes

.pre-commit-hooks.yaml Show resolved Hide resolved

mwouts reviewed Jan 11, 2021

View reviewed changes

JohnPaton added 3 commits January 18, 2021 16:31

Try calling pre_comit directly

08b2beb

When in doubt, return False

1814e4c

Fix exisiting file test

399da32

Only update the timestamp (not the content) of the text input file

26ea640

JohnPaton added 4 commits January 20, 2021 16:25

Only run on notebooks

c154e01

use arrays

8a9d246

Only run on notebooks

d3ca629

Remove additional dependency

35055a1

JohnPaton added 2 commits January 20, 2021 16:36

Handle pre-commit not being installed

dea5704

Relevant tests will be skipped in that case

Handle right exception

caedd61

JohnPaton marked this pull request as ready for review January 20, 2021 16:22

JohnPaton requested a review from mwouts January 20, 2021 16:23

mwouts merged commit 41b7e9c into mwouts:master Jan 21, 2021

This was referenced Jan 21, 2021

Add more tests on the pre-commit hook #722

Closed

Add a .pre-commit-hooks.yaml to enable hooks to refer to this repo #691

Closed

mwouts mentioned this pull request Mar 7, 2021

jupytext in pre-commit hook #292

Closed

mwouts mentioned this pull request Oct 19, 2021

Pre commit hook workflow #871

Open

Enable integration with pre-commit #698

Enable integration with pre-commit #698

Conversation

JohnPaton commented Dec 30, 2020

Skylion007 commented Jan 6, 2021 • edited Loading

Skylion007 commented Jan 6, 2021 • edited Loading

Skylion007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnPaton Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnPaton Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skylion007 Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Skylion007 Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwouts commented Jan 6, 2021

Skylion007 commented Jan 7, 2021

mwouts commented Jan 8, 2021

Choose a reason for hiding this comment

JohnPaton Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Skylion007 left a comment

Choose a reason for hiding this comment

JohnPaton commented Jan 8, 2021

JohnPaton commented Jan 9, 2021 • edited Loading

mwouts commented Jan 10, 2021

JohnPaton commented Jan 11, 2021

JohnPaton commented Jan 11, 2021

mwouts commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnPaton commented Jan 18, 2021

mwouts commented Jan 18, 2021

Skylion007 commented Jan 19, 2021 via email

mwouts commented Jan 19, 2021

JohnPaton commented Jan 19, 2021 • edited Loading

Skylion007 commented Jan 19, 2021 • edited Loading

mwouts commented Jan 19, 2021

JohnPaton commented Jan 20, 2021

mwouts commented Jan 20, 2021

JohnPaton commented Jan 20, 2021

mwouts commented Jan 20, 2021

Skylion007 commented Jan 20, 2021 • edited Loading

mwouts commented Jan 21, 2021

Skylion007 commented Jan 6, 2021 •

edited

Loading

Skylion007 commented Jan 6, 2021 •

edited

Loading

JohnPaton Jan 8, 2021 •

edited

Loading

JohnPaton Jan 8, 2021 •

edited

Loading

Skylion007 Jan 8, 2021 •

edited

Loading

Skylion007 Jan 6, 2021 •

edited

Loading

JohnPaton Jan 12, 2021 •

edited

Loading

JohnPaton commented Jan 9, 2021 •

edited

Loading

JohnPaton commented Jan 19, 2021 •

edited

Loading

Skylion007 commented Jan 19, 2021 •

edited

Loading

Skylion007 commented Jan 20, 2021 •

edited

Loading