pre-commit #29
Comments
|
Happy to support this if needed 👍 |
|
That's awesome! I just updated the docs at https://github.com/restyled-io/restyled.io/wiki/Adding-a-Restyler to (hopefully) be up to date and ready to follow. If you run into any bumps trying to add it please reach out. |
|
ok I ran into a bunch of things I first wanted to validate the setup by building and running one of the other fixers. I took First because I am severely bandwidth constrained right now, I switched that dockerfile to the diff --git a/autopep8/Dockerfile b/autopep8/Dockerfile
index f351cc5..6e0b0fc 100644
--- a/autopep8/Dockerfile
+++ b/autopep8/Dockerfile
@@ -1,4 +1,4 @@
-FROM python:3.6
+FROM python:3.6-slim
MAINTAINER Pat Brisbin <[email protected]>
ENV LANG en_US.UTF-8
RUN pip install autopep8(this shouldn't make a difference, but saves me ~100MB of bandwidth) I then tried to run the tests... first thing I hit is the use of cram "test/autopep8.t"
!
--- test/autopep8.t
+++ test/autopep8.t.err
@@ -1,30 +1,9 @@
$ source "$TESTDIR/helper.sh"
- If you don't see this, setup failed
+ /bin/sh: 2: source: not found
+ [127]$ sh -c 'source /dev/null'
sh: 1: source: not foundHowever $ sh -c '. /dev/null'
$So I switched the test to use that (let me know if you'd like a PR I can switch all of them at once) diff --git a/test/autopep8.t b/test/autopep8.t
index 173ecb4..bd3d0e5 100644
--- a/test/autopep8.t
+++ b/test/autopep8.t
@@ -1,4 +1,4 @@
- $ source "$TESTDIR/helper.sh"
+ $ . "$TESTDIR/helper.sh"
If you don't see this, setup failed
autopep8I then hit a weirdness with the diff outputs (which used @@ -2,7 +2,7 @@
If you don't see this, setup failed
autopep8
$ run_restyler autopep8 crazy.py
- diff --git i/crazy.py w/crazy.py
+ diff --git a/crazy.py b/crazy.py
index 9941de7..6451f03 100644
@@ -8,6 +8,6 @@
index 9941de7..6451f03 100644
- --- i/crazy.py
- +++ w/crazy.py
+ --- a/crazy.py
+ +++ b/crazy.py
@@ -1,9 +1,12 @@
-import math, sys;
+import mathgreat, after that the autopep8 test passed: $ make autopep8/Dockerfile.tested
...
cram "test/autopep8.t"
.
# Ran 1 tests, 0 skipped, 0 failed.
echo > autopep8/Dockerfile.tested
rm autopep8/Dockerfile.built
$ echo $?
0So then I sought out to write one for pre-commit. pre-commit depends on a configuration ( So I wrote a dockerfile, some metadata, and a test: FROM ubuntu:bionic
RUN : \
&& apt-get update \
&& DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
ca-certificates \
dumb-init \
gcc \
git \
python3.7-dev \
virtualenv \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*
ARG VERSION=1.17.0
ENV PATH=/venv/bin:$PATH
RUN virtualenv /venv -ppython3.7 && pip install "pre-commit==$VERSION"
WORKDIR /code
ENTRYPOINT ["dumb-init", "--"]---
name: pre-commit
image: restyled/pre-commit:v1.17.0
command: [pre-commit, run, --files]
arguments: []
include: ['**/*']
interpreters: []
supports_arg_sep: false
supports_multiple_paths: true
documentation: ['https://pre-commit.com']
metadata:
languages: [All]In this case I used repos:
- repo: https://github.com/python/black
rev: 19.3b0
hooks:
- id: blackdef very_important_function(template: str, *variables, file: os.PathLike, engine: str, header: bool = True, debug: bool = False):
"""Applies `variables` to the `template` and writes to `file`."""
with open(file, 'w') as f:
...I then ran my test and noticed that it was not copying in the hidden file into the fixture git directory. So I applied this patch (let me know if you'd like a PR for this as well) diff --git a/test/helper.sh b/test/helper.sh
index afe15e2..3860b29 100644
--- a/test/helper.sh
+++ b/test/helper.sh
@@ -11,7 +11,7 @@ set -e
mkdir repo
cd repo
-cp "$TESTDIR"/fixtures/* .
+cp -r "$TESTDIR"/fixtures/. .
{
git initAfter that I ran my test yet again and it looks like I've hit an intentional limitation: cram --keep-tmpdir "test/pre-commit.t"
!
--- test/pre-commit.t
+++ test/pre-commit.t.err
@@ -4,20 +4,16 @@
pre-commit
$ run_restyler pre-commit black_example.py
- diff --git i/black_example.py w/black_example.py
- --- test/fixtures/black_example.py.orig 2019-07-03 07:31:06.595000999 -0700
- +++ test/fixtures/black_example.py 2019-07-03 07:31:20.263000999 -0700
- @@ -1,4 +1,11 @@
- -def very_important_function(template: str, *variables, file: os.PathLike, engine: str, header: bool = True, debug: bool = False):
- +def very_important_function(
- + template: str,
- + *variables,
- + file: os.PathLike,
- + engine: str,
- + header: bool = True,
- + debug: bool = False
- +):
- """Applies `variables` to the `template` and writes to `file`."""
- - with open(file, 'w') as f:
- + with open(file, "w") as f:
- ...
+ [INFO] Initializing environment for https://github.com/python/black.
+ An unexpected error has occurred: CalledProcessError: Command: ('/usr/bin/git', 'fetch', 'origin', '--tags')
+ Return code: 128
+ Expected return code: 0
+ Output: (none)
+ Errors:
+ fatal: unable to access 'https://github.com/python/black/': Could not resolve host: github.com
+
+
+ Check the log at /root/.cache/pre-commit/pre-commit.log
+ /home/asottile/workspace/restylers/test/../build/restyler-meta:29:in `run': ["docker", "run", "--rm", "--net", "none", "--volume", "/tmp/cramtests-2b1cwxvz/pre-commit.t/repo:/code", "restyled/pre-commit:v1.17.0", "pre-commit", "run", "--files", "black_example.py"] failed (RuntimeError)
+ \tfrom /home/asottile/workspace/restylers/test/../build/restyler-meta:68:in `run' (esc)
+ \tfrom /home/asottile/workspace/restylers/test/../build/restyler-meta:83:in `<main>' (no-eol) (esc)
# Ran 1 tests, 0 skipped, 1 failed.
# Kept temporary directory: /tmp/cramtests-2b1cwxvz
Makefile:13: recipe for target 'pre-commit/Dockerfile.tested' failed
make: *** [pre-commit/Dockerfile.tested] Error 1
rm pre-commit/Dockerfile.builtthat is, |
|
Awesome stuff! Really great work getting through all that, sorry it wasn't totally non-trivial. Here are my thoughts on each thing you raised:
I would probably accept a patch to make the tests POSIX, but there were some historical reasons that I needed bash, so it's possible something could crop up later when we'll want that again. For this reason, I would probably lean towards updating the
Oh crap. This is a
I think for this case we would put a file in
I think if we resolve the previous point, this won't be needed. But including hidden files in the
Yeah... That's kind of stuck that way. I just can't give up |
|
Cool cool, sounds like Would you be ok with me doing the following things:
And perhaps as well after that:
|
|
oh I see you're using circle, but can't run the tests -- switch to travis or azure pipelines maybe? |
|
Thanks very much, I have some thoughts on these, but probably won't be able
to get back to a computer for a proper reply until after the weekend.
…On Wed, Jul 3, 2019, 18:54 Anthony Sottile ***@***.***> wrote:
oh I see you're using circle, but can't run the tests -- switch to travis
or azure pipelines maybe?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#29?email_source=notifications&email_token=AAAMM7CHDVNLC7E7IVPL7HLP5UUZHA5CNFSM4H455Z4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZF4APQ#issuecomment-508280894>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAMM7G7BBGLXDR33IO3LF3P5UUZHANCNFSM4H455Z4A>
.
|
|
OK, back at a computer so I can give that proper reply now :)
I'd like to understand this better: in-repository configuration is totally fine and expected. What we can't allow is downloading hooks/plugins from the open internet as part of restyling. But couldn't we pre-install any plugins into the Restyler image ahead of time?
Do you know if Travis or Azure would resolve the core limitation that we need to mount files local to where the tests are running into the docker container?
Yes please :)
👍 I have this change locally, but you can just batch it in with your other fixes.
Doh. 👍
Yes please :) -- I have an Issue for I'm also still pondering how to make it easy to add lots of individual Restylers if they're all mostly the same -- such as |
|
I've picked this up and made some progress in #553, please see the commit message for more details. As of right now, it successfully builds a restyler image with two revisions of the @asottile would you be willing to re-engage on this and see if we can get this to a useful state? |
|
this won't work unless you install every possible permutation I made https://pre-commit.ci which kinda does this way better |
|
I would say,
Are you saying even that lower bar is not feasible?
Yes, I suppose it seems like two means to the same end: if every auto-formatter had a corresponding Restyler image and pre-commit hook, then I guess it is more valuable to look at pre-commit hooks that are not yet Restylers and focus on making them individually, rather than trying to run them via pre-commit. |
|
even if you got the "common" versions today the tools all evolve and gain features. for example it'd be difficult-to-impossible to keep a ruff Restyler up to date with pre-commit.ci the things it runs in CI are precisely what is configured locally in the dev tooling so there's no chance of drift and no need to set up every tool manually and keep it in sync somehow. pre-commit also covers tools that would never have a Restyler (org specific tools, rare tooling, repository specific tools, etc.) |
Meh, not really. If this were "impossible", Restyled wouldn't be useful or successful at all. All of our Restylers are wrapping some version of an in-the-wild auto-formatter, and that version needs maintaining as they evolve. I'm not saying it's easy or totally solved, but we do alright keeping up. Most Restylers are automated through Renovate PRs.
Makes sense. These features are only possible because you allow network requests to download things on-demand. Code, that is Remote, that you then Execute... You can see where I'm going. I assume you've got some effective mitigations, but at the end of the day you have to download and execute code that is identified in user input to provide those features; Restyled has just chosen not to cross that line. Anyway, thanks for discussing. Sounds like there's no need or no interest in a pre-commit Restyler. |
(Original Issue: restyled-io/restyled.io#164)
pre-commit (https://pre-commit.com/) is a multi-language package manager for pre-commit hooks which are typically used to format code.
Adding pre-commit support to restyled would leverage the large ecosystem of formatters that pre-commit already supports.
See past discussion of this idea here: pre-commit/pre-commit.com#211
The text was updated successfully, but these errors were encountered: