Skip to content

Conversation

@gwennlbh
Copy link
Contributor

Add a pre-commit hook to generate a graph of dependencies, see https://pre-commit.com/#new-hooks

Add a pre-commit hook to generate a graph of dependencies, see https://pre-commit.com/#new-hooks
@coveralls
Copy link

coveralls commented Jul 24, 2020

Coverage Status

Coverage decreased (-0.1%) to 79.51% when pulling a7e6e9b on ewen-lbh:patch-1 into e56aac1 on thebjorn:master.

@gwennlbh
Copy link
Contributor Author

The CI errors seem unrelated to my commit, except for the coverage decrease, of which YAML files should be ignored, it seems weird to me that YAML files are taken into account when calculating the project coverage.

@thebjorn
Copy link
Owner

Hi Ewen, and thank you for your interest in pydeps. Could you say a few word about the motivation for this PR (what this PR accomplishes)?

@gwennlbh
Copy link
Contributor Author

This PR allows to use the executable (pydeps) as a step in pre-commit, so that the dependency graph is always up-to-date with the codebase at the commit level

@thebjorn
Copy link
Owner

@ewen-lbh is this so other projects can use it, or for pydeps itself?

@gwennlbh
Copy link
Contributor Author

@thebjorn it's for other projects to use

@thebjorn thebjorn merged commit 5462e6e into thebjorn:master Jul 24, 2020
@gwennlbh
Copy link
Contributor Author

gwennlbh commented Jul 24, 2020

So it seems like I forgot to make the file declare an array instead of a hook object directly, can @thebjorn do it directly, do I add commits to this branch or do I open a new PR?

Sorry, I should've tested this beforehand with a dummy repo

@thebjorn
Copy link
Owner

@ewen-lbh Let me know what the correct contents of the file should be and I'll change it.

@gwennlbh
Copy link
Contributor Author

gwennlbh commented Jul 24, 2020

@thebjorn just putting all the keys into an array should work, sth. like:

- id: ...
  name: ...
  entry: ...

But I'll test it on a dummy fork tomorrow just to be sure and not make a mistake twice 😅

@thebjorn
Copy link
Owner

That change makes the hook valid, but it still doesn't work automatically (since pydeps takes the name of the directory to run against as an argument). Probably just me not knowing how to use pre-commit though.

(dkformgen) go|c:\srv\lib\dkformgen> pre-commit try-repo --verbose ../code/pydeps pydeps
[WARNING] Creating temporary repo with uncommitted changes...
===============================================================================
Using config:
===============================================================================
repos:
-   repo: c:\users\bjorn\appdata\local\temp\tmpjuxkxn\shadow-repo
    rev: f0bd85b93a45a4bad4b35a8275bfd238e3922d08
    hooks:
    -   id: pydeps
===============================================================================
[INFO] Initializing environment for c:\users\bjorn\appdata\local\temp\tmpjuxkxn\shadow-repo.
[INFO] Installing environment for c:\users\bjorn\appdata\local\temp\tmpjuxkxn\shadow-repo.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Generate a graph of dependencies.....................(no files to check)Skipped
- hook id: pydeps

(dkformgen) go|c:\srv\lib\dkformgen>

@thebjorn
Copy link
Owner

I've added a --find-package flag which will run pydeps from the directory where setup.py is, however, this fails when pydeps is passed the list of committed files:

(dkformgen) go|c:\srv\lib\dkformgen> cat .pre-commit-config.yaml
repos:
    - repo: https://github.com/thebjorn/pydeps
      rev: v1.9.5
      hooks:
        - id: pydeps

output:

(dkformgen) go|c:\srv\lib\dkformgen> git commit -m "add pre-commit hook"
[INFO] Initializing environment for https://github.com/thebjorn/pydeps.
[INFO] Installing environment for https://github.com/thebjorn/pydeps.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Generate a graph of dependencies.........................................Failed
- hook id: pydeps
- exit code: 2

usage: pydeps [-h] [--debug] [--config FILE] [--no-config] [--version]
              [-L LOG] [--find-package] [--fname FNAME] [-v] [-o file]
              [-T FORMAT] [--display PROGRAM] [--noshow] [--show-deps]
              [--show-raw-deps] [--show-dot] [--nodot] [--no-output]
              [--show-cycles] [--debug-mf INT] [--noise-level INT]
              [--max-bacon INT] [--pylib] [--pylib-all] [--include-missing]
              [-x PATTERN [PATTERN ...]] [-xx MODULE [MODULE ...]]
              [--only MODULE_PATH [MODULE_PATH ...]] [--externals] [--reverse]
              [--cluster] [--min-cluster-size INT] [--max-cluster-size INT]
              [--keep-target-cluster] [--rmprefix PREFIX [PREFIX ...]]
              [--start-color INT]
pydeps: error: unrecognized arguments: .pre-commit-config.yaml

It's getting late, I'll have to pick this up tomorrow...

@gwennlbh
Copy link
Contributor Author

gwennlbh commented Jul 25, 2020

@thebjorn Users can define arguments to pass to entry, so the hook is intended to be used that way:

in ./.pre-commit-config.yaml:

repos:
    - repo: https://github.com/thebjorn/pydeps
      rev: (set the version tag "vVERSION" here)
      hooks:
          - id: pydeps
            args: [args, to, pass, to, pydeps]

A way to detect the package name from setup.{py,cfg} or pyproject.toml would be a nice little QoL change but it's not the end of the world to have users of the pre-commit hook need to specify the module name, and I'm sure plenty of other hooks require some sort of configuration via arguments. That said, I'm still pretty new to pre-commit.com

@thebjorn
Copy link
Owner

The problem is that pre-commit passes the filenames that have changed, which is unexpected. From the example above, I checked in the .pre-commit-config.yaml file, and it was passed to pydeps as an argument (in addition to the --find-package argument I've defined in pydeps. That makes pydeps' argparser very confused. The two solutions I see are (1) develop functionality that does something useful for the files that are passed in, (2) change the argparser so it ignores filenames when --find-package is passed.
I'm leaning towards number 2 being more useful...

@gwennlbh
Copy link
Contributor Author

Yes, the spirit of pre-commit hooks seems to be that each hook is applicable to individual files (so that checks can also be skipped when the commit does not change files that are affected by a hook). Overall I feel like things like poetry export (to generate a requirements.txt file) or pydeps are "global" actions executed at each commit (even if pydeps could be run only if a python file had imports changed)

Seems like this kind of programs are outside of pre-commit.com's vision, at least it seems so.

But still, having an always-up-to-date dependency graph is nice

Maybe a different CLI (something like pydeps-hook) altogether would be more appropriate, as the current solutions you proposed all seem pretty hacky to me...

I may develop that CLI :)

Or another idea, building upon your (1):
If files are passed, check if those files' imports have changed since the last commit, and if none have had imports that were modified, don't bother re-generating the graph

But this seems a bit complex to implement at first glance

@thebjorn
Copy link
Owner

Work has intruded on my free time, so I'm putting this on the back burner for now. Keep me updated if you develop pydeps-hook :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants