Add option `--do-not-add` to `sync-hooks-additional-dependencies` #11

souliane · 2024-04-10T10:31:48Z

Closes #8

Successful PR Checklist:

Tests
Documentation

PR label(s):

souliane · 2024-04-10T10:33:06Z

README.md

@@ -47,7 +47,8 @@ repos:
        # "mypy" is the id of a pre-commit hook
        # "types" is the name of your poetry group containing typing dependencies
        # "main" is the automated name associated with the "default" poetry dependencies
-        args: ["--bind", "mypy=types,main"]
+        # `--do-not-add` will update or remove packages, but not add any new one.
+        args: ["--bind", "mypy=types,main", "--do-not-add"]


Not sure if you want this here. I added it because it seems that this sample config lists all possible option.

README.md

Closes ewjoachim#8

ewjoachim

Here's a first quick round of review, good job so far :) A few comments, though.

tests/test_sync_hooks_additional_dependencies.py

ewjoachim · 2024-04-17T21:54:21Z

README.md

@@ -47,7 +47,8 @@ repos:
        # "mypy" is the id of a pre-commit hook
        # "types" is the name of your poetry group containing typing dependencies
        # "main" is the automated name associated with the "default" poetry dependencies
-        args: ["--bind", "mypy=types,main"]
+        # `--do-not-add` will update or remove dependencies, but not add any new one.
+        args: ["--bind", "mypy=types,main", "--do-not-add"]


I'm not a huge fan of negative options, and I if we do it, I prefer --no-x, I think I linked update better, but I understand you changed it because it could remove deps too.
What would you say of --no-new-deps ?

Done with --no-new-deps (which is better indeed).
And yes, I initially suggested --update-only but I then realized that it does not make sense to keep dependencies that have been removed from poetry, so I had to find another name.

README.md

ewjoachim · 2024-04-17T22:03:55Z

poetry_to_pre_commit/sync_hooks_additional_dependencies.py

+    # Additional packages that are already in pre-commit configuration could be listed with
+    # any format that is accepted by pip. The following regex might not cover all the cases.
+    current_deps = re.findall(
+        r"^\s*([^=><\[\s]+)", "\n".join(hook_additional_deps), re.MULTILINE
+    )


I believe packaging.requirements.Requirement is a much more robust solution. This project is partially born from not wanting to use regex parsing for pre-commit config, so I'm much more in favor of proper parsing, even it if means one more dependency.

Done - much better! Also this was already an indirect dependency, so we basically got it for free :-)

souliane · 2024-04-18T09:31:47Z

poetry_to_pre_commit/sync_hooks_additional_dependencies.py

+        package
+        for package in poetry_deps
+        # package is yielded by `get_poetry_deps` above, and we are pretty sure that this won't raise `IndexError`
+        if package.split("==")[0].split("[")[0] in current_deps


This package.split is quite ugly. I don't know what else to do without modifying get_poetry_deps, but if you are OK I would to make it yield Dependency object instead of strings. Basically, to replace:

yield f"{dep.complete_name}=={package.version}"

with:

yield dep

So that I can do package.name here.

I think the problem is that what we pass to this function does not fit anymore what we need.

We should recieve a set of PoetryPackages and that class should be updated to also have optional extras (set[str]). There should be a function (or a class of PoetryPackage) that turns a PoetryPackage into a requirement string (sort the extras by name to ensure consistency).
This way, we can filter the PoetryPackages using current_deps (which should be a set rather than a list).

OK, I realize that my suggestion to yield dep would not work, because we then don't have the pinned version.
I started to refactor following your suggestions, but this is actually too much changes for this PR. This requires to adapt several things including the tests because set and PoetryPackage are not hashable etc. I would be OK to do this in another PR.

PS: have you considered to organize your hooks with classes? I find it easier to read than several functions, because it allows to scope them together + shorten the function names (since we already get the common info from the class). But I guess this is a matter of personal preference and don't want to debate that :-) It's just a suggestion. I know that you prefer to avoid classes in the test files, but I was wondering if you would like them in the actual code.

Ok... So I'm a bit lost: is the PR in a state you'd consider mergeable ?

Well the package.split are very ugly... It would be cleaner to refactor with PoetryPackage first, if you like I will create another MR (and then rebase this one). Or you can merge this and we shall quickly clean it, as you prefer :-)

Let's merge, then :)
The stakes are still quite low, so...

ewjoachim

Woo, getting better and better !

At any point, fell free to say if you'd rather I finish it, given I'm suggesting a lot of small "nitpick" points here :)

ewjoachim · 2024-04-19T22:24:50Z

poetry_to_pre_commit/sync_hooks_additional_dependencies.py

+        package
+        for package in poetry_deps
+        # package is yielded by `get_poetry_deps` above, and we are pretty sure that this won't raise `IndexError`
+        if package.split("==")[0].split("[")[0] in current_deps


I think the problem is that what we pass to this function does not fit anymore what we need.

We should recieve a set of PoetryPackages and that class should be updated to also have optional extras (set[str]). There should be a function (or a class of PoetryPackage) that turns a PoetryPackage into a requirement string (sort the extras by name to ensure consistency).
This way, we can filter the PoetryPackages using current_deps (which should be a set rather than a list).

poetry_to_pre_commit/sync_hooks_additional_dependencies.py

ewjoachim · 2024-04-19T22:32:03Z

tests/test_sync_hooks_additional_dependencies.py

+        (["a==1", "b"], ["a>=1"], ["a==1"]),
+    ],
+)
+def test_sync_hook_additional_deps_with_no_new_deps(


Wow, I hadn't noticed how sync_hook_additional_deps and sync_hooks_additional_dependencies were badly named. You don't have to do this, but if you want to rename either, please feel free.

Also, in test function names, I like using __ to split the name of the function under test and the description of the test.

No worries. I renamed for now sync_hook_additional_deps to _sync_hooks_additional_dependencies but this is probably not what you had in mind :-p Please tell me which names you prefer.

I like using __ to split the name [...]

Done. I also added __ in test__sync_hooks_additional_dependencies, which looks a bit strange... I will change it together with the new names.

this is probably not what you had in mind :-p

Honnestly it's already much better than what it was. At list it's clear which one is the outer "public" one and which one is the inner. We'll find better names but it doesn't have to be in this PR so let it live this way. Thank you :)

souliane requested a review from ewjoachim as a code owner April 10, 2024 10:31

souliane force-pushed the main branch from ddb81aa to dccf73e Compare April 10, 2024 10:35

souliane commented Apr 10, 2024

View reviewed changes

Add option --do-not-add to sync-hooks-additional-dependencies

fa7163f

Closes ewjoachim#8

souliane force-pushed the main branch from dccf73e to fa7163f Compare April 10, 2024 10:42

ewjoachim reviewed Apr 17, 2024

View reviewed changes

souliane added 2 commits April 18, 2024 11:10

rename --do-no-add to --no-new-deps + no test class

8e29717

use packaging.requirement.Requirement to parse deps

0e6412f

souliane commented Apr 18, 2024

View reviewed changes

ewjoachim reviewed Apr 19, 2024

View reviewed changes

code style improvements

33a16f0

ewjoachim merged commit e7e4850 into ewjoachim:main May 3, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option `--do-not-add` to `sync-hooks-additional-dependencies` #11

Add option `--do-not-add` to `sync-hooks-additional-dependencies` #11

souliane commented Apr 10, 2024

souliane Apr 10, 2024

ewjoachim left a comment

ewjoachim Apr 17, 2024

souliane Apr 18, 2024

ewjoachim Apr 17, 2024

souliane Apr 18, 2024

souliane Apr 18, 2024 •

edited

Loading

ewjoachim Apr 19, 2024

souliane May 2, 2024

ewjoachim May 2, 2024 •

edited

Loading

souliane May 3, 2024

ewjoachim May 3, 2024

ewjoachim left a comment •

edited

Loading

ewjoachim Apr 19, 2024

ewjoachim Apr 19, 2024

souliane May 2, 2024

ewjoachim May 2, 2024

Add option --do-not-add to sync-hooks-additional-dependencies #11

Add option --do-not-add to sync-hooks-additional-dependencies #11

Conversation

souliane commented Apr 10, 2024

Successful PR Checklist:

PR label(s):

Choose a reason for hiding this comment

ewjoachim left a 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

souliane Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewjoachim May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewjoachim left a comment • 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

Add option `--do-not-add` to `sync-hooks-additional-dependencies` #11

Add option `--do-not-add` to `sync-hooks-additional-dependencies` #11

souliane Apr 18, 2024 •

edited

Loading

ewjoachim May 2, 2024 •

edited

Loading

ewjoachim left a comment •

edited

Loading