Add just and pre-commit #201

sarayourfriend · 2021-09-07T17:10:31Z

Fixes

Fixes #193 by @sarayourfriend
Fixes #187 by @sarayourfriend

Description

Adds pre-commit using the same configuration as the openverse-catalog with some modifications to ignore rules in certain files that were problematic.

Also adds a justfile to make docker stuff easier.

Technical details

I had to add some noqa comments for long strings (I wish flake8 would ignore strings...)

I also added .flake8 that mirrors the pre-commit configuration for flake8 so that editor integration will work

Tests

Checkout the branch and run just install then pipenv run pre-commit run --all-files and ensure that there are no errors reported.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main or master).
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

dhruvkb · 2021-09-08T13:50:09Z

@sarayourfriend we should remove the 'Style' job from the 'Automated tests' workflow as the new 'Linting' job supersedes it (the Flake8 check in 'Linting' already includes the pycodestyle check from 'Style'). Also that's the only failing check in a PR that otherwise LGTM.

sarayourfriend · 2021-09-08T13:55:40Z

Oops, I meant to remove it when I added the linting job 😅 It's gone now, thanks!

AetherUnbound

Looks great! I only have some additional thoughts on the justfile

justfile

krysal

It looks pretty good! I just leave some suggestions and questions to evaluate.

.github/workflows/integration-tests.yml

.pre-commit-config.yaml

README.md

ingestion_server/ingestion_server/es_mapping.py

krysal · 2021-09-13T17:12:21Z

README.md

-2. Run the tests in a Pipenv subshell.
-```
-pipenv run bash ./test/run_test.sh
+just testlocal


I got this by trying to run this command:

error: Justfile does not contain recipe `testlocal`

Oh hmm I removed it for some reason. Let me see.

Do you know why these tests must run in a "pipenv subshell"?

Edit: Oh never mind, I see that the script calls to pytest.

This is going to throw a wrench in the root Pipfile idea 😞

Personally, I run the tests inside the container, we can leave only those instructions for tests.

docker-compose exec web bash ./test/run_test.sh

I wonder if it's ever necessary to run it outside of the container 🤔

Sometimes, I run the code outside the container when debugging it line by line (I like PyCharm's debugger GUI). Running the tests outside the containers helps in that scenario. I'm open to a different process though if I'm the only one using it like that.

Yes, Pycharm can take the interpreter of a container, it's quite flexible in this aspect. However, I can see the value in the local approach in not being tied up to this particular IDE, neither to the use of just if that is what is blocking here.

@krysal from what I had read in the docs, PyCharm can work with a Docker environment only if it created the container. I wasn't able to get it to connect with the existing web container orchestrated by Docker Compose 😢.

Hmm, I don't think that's true Dhruv, the docs might be incorrect. At my previous job we were able to get it to connect to the docker container just fine and it was running using docker compose... but maybe things have changed since then.

In any case, after making the changes in the latest commits we're able to add back the testlocal script 🎉

The testlocal script works perfectly. So does the regular test one.

dhruvkb · 2021-09-15T07:36:02Z

@sarayourfriend this might be a silly suggestion but I was playing around with pre-commit in the current setup and came across something that might be a good compromise.

Let's say we installed pre-commit in both the existing Pipfiles while keeping the .pre-commit-config.yaml in the repo root. To set up the hooks we go into either of the repos and invoke pipenv run pre-commit install.

The hook created records the virtualenv that was active when install was invoked so there's no need to always activate the virtualenv when committing. All pre-commit hooks manage their own environment so there's no need for installing black or flake8 on any project Pipfile.

Apart from having to list pre-commit in two Pipfiles, what do you think are the cons of this approach?

sarayourfriend · 2021-09-15T14:05:57Z

Apart from having to list pre-commit in two Pipfiles, what do you think are the cons of this approach?

The real cons are for editor integration, but we're already sacrificing the best of editor integration (dependency knowledge) in the current set up, so I think what you propose is better.

We can add it to both, but just install should only install one right?

dhruvkb · 2021-09-15T19:02:48Z

Let's use just install to install from the openverse-api directory. That's the main project in the repo (it shouldn't matter but still)

Co-authored-by: Krystle Salazar <krystle.salazar@ciens.ucv.ve>

sarayourfriend · 2021-09-16T15:45:40Z

@dhruvkb I did as you suggested and it works great 🙂

dhruvkb

To finalise the changes, it'll be good to squash the commits in such a way that

all formatting changes are squashed into one commit to avoid ruining Git blame
all config changes are squashed into one commit (to make reviewing the PR easier)

dhruvkb · 2021-09-16T17:51:50Z

.pre-commit-config.yaml

+          - --ensure-newline-before-comments
+          - --line-length=88
+
+  - repo: https://gitlab.com/pycqa/flake8


~~Just to match the official way their org is named.~~

Their docs show a GitHub URL. Let's use that.

Suggested change

- repo: https://gitlab.com/pycqa/flake8

- repo: https://github.com/PyCQA/flake8

dhruvkb · 2021-09-16T17:56:03Z

I wonder why the tests are still failing. I'll pull this locally to review so will also look into that then.

sarayourfriend · 2021-09-16T18:08:26Z

@dhruvkb Sure, I can do that. Alternatively I could also merge using the squash and merge strategy so it will all be under one commit. Do you have a strong preference either way?

dhruvkb · 2021-09-16T18:19:40Z

I have no hard preferences, whatever is convenient for you is cool.

dhruvkb

I'll review this tomorrow.

.github/workflows/integration-tests.yml

dhruvkb

A linter, formatter and Git hook trio is a much needed addition to the repo! Thanks for doing this.

And with passing tests, I have a lot of confidence in this PR. Apart from all the breaking changes this'll cause in all other PRs (especially the big one #194), LGTM.

.flake8

dhruvkb · 2021-09-17T07:12:40Z

.pre-commit-config.yaml

+          - --ensure-newline-before-comments
+          - --line-length=88
+
+  - repo: https://gitlab.com/pycqa/flake8


Their docs show a GitHub URL. Let's use that.

Suggested change

- repo: https://gitlab.com/pycqa/flake8

- repo: https://github.com/PyCQA/flake8

.pre-commit-config.yaml

dhruvkb · 2021-09-17T07:15:33Z

.pre-commit-config.yaml

+            - --per-file-ignores=*test*:E501,*__init__*:F401,*wsgi.py:E402
+            - --max-line-length=88
+            - --ignore=E203,W503


I think these args won't be necessary as the hook will automatically use the config from .flake8.

I won't, unfortunately. pre-commit runs hooks inside a virtualenv that doesn't include the root of your project, so it doesn't know any of your configuration files 😢

I tried this locally and it seemed to work fine. Are you sure about it not working?

@sarayourfriend for the hook, is it possible to provide a config path instead of defining the rules twice in two different locations? If not I think just leaving a note here should be enough that mentions the rules also being in the other file.

I guess I'm not sure 😅 I'd tried it in the past and it didn't work and I found some SO answers about it to confirm the issue, but if you've got it working I'll try it again!

Ah it worked! I removed the duplicated config. Thanks for testing it out!

README.md

dhruvkb · 2021-09-17T07:18:15Z

README.md

-2. Run the tests in a Pipenv subshell.
-```
-pipenv run bash ./test/run_test.sh
+just testlocal


The testlocal script works perfectly. So does the regular test one.

justfile

dhruvkb · 2021-09-17T07:26:20Z

@sarayourfriend is there a possibility of holding the merge for this PR and merging other open PRs first? It'll be very hard to update them with main after this is merged, while this one will be easier to update if we just took the configs and re-ran the hooks.

dhruvkb

There are some indentation inconsistencies in the pre-commit config. Is it weird that pre-commit didn't catch this?

.pre-commit-config.yaml

Co-authored-by: Dhruv Bhanushali <dhruv_b@live.com>

sarayourfriend · 2021-09-17T14:06:52Z

is there a possibility of holding the merge for this PR and merging other open PRs first

Sure! I suppose it'd be easier to just let the repo settle down at some point and then redo this PR from scratch... the merge conflicts will be difficult to resolve either way (usually these types of PRs are best applied quickly before too much work is done in the meantime, but this one has taken longer 😞) so it'll be easy to just copy the configuration from this PR and then run the formatting.

I guess can you let me know when you think this PR should be merged? Otherwise we'll be waiting forever for work to settle 🙂 At some point we just have to accept the complications.

sarayourfriend · 2021-09-17T14:14:11Z

Oh also, indentation is fixed! Thanks for pointing it out.

zackkrida · 2021-09-19T23:28:52Z

How about we merge this after these are merged/closed?

dhruvkb · 2021-09-20T01:27:02Z

#202 will need to be reworked anyway, we can merge that later. But #194 is too big to redo 😢.

sarayourfriend · 2021-09-21T20:52:02Z

Closing in favor of #224

sarayourfriend requested a review from a team as a code owner September 7, 2021 17:10

sarayourfriend requested review from krysal and obulat September 7, 2021 17:10

dhruvkb mentioned this pull request Sep 9, 2021

Refactor the use of environment variables #202

Merged

7 tasks

AetherUnbound approved these changes Sep 13, 2021

View reviewed changes

justfile Outdated Show resolved Hide resolved

justfile Outdated Show resolved Hide resolved

krysal reviewed Sep 13, 2021

View reviewed changes

.github/workflows/integration-tests.yml Outdated Show resolved Hide resolved

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

README.md Outdated Show resolved Hide resolved

ingestion_server/ingestion_server/es_mapping.py Outdated Show resolved Hide resolved

krysal reviewed Sep 13, 2021

View reviewed changes

zackkrida assigned sarayourfriend Sep 14, 2021

zackkrida added 🟩 priority: low Low priority and doesn't need to be rushed 🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Sep 14, 2021

sarayourfriend and others added 12 commits September 16, 2021 08:43

Add pre-commit

abf1f12

Run pre-commit an all files

b24a10a

Use sh instead of bash

fa5d261

Add flake8 configuration for editor integration

d2251db

Use pre-commit for linting on CI

1ef2741

Add flake8 and black for editor dependencies

974e572

Remove duplicated style job

98c4c32

Condense logs into a single parameterized command

abf3e52

Add link to just and clarify that docker-compose still works

58956d7

Co-authored-by: Krystle Salazar <krystle.salazar@ciens.ucv.ve>

Disable formatting for es_mapping.py

7bea78a

Use latest versions of actions

27a402c

Co-authored-by: Krystle Salazar <krystle.salazar@ciens.ucv.ve>

Move pre-commit and dev deps into openverse-api Pipfile

30ccacc

sarayourfriend force-pushed the add/pre-commit branch from 8813741 to 30ccacc Compare September 16, 2021 15:45

sarayourfriend added 4 commits September 16, 2021 08:50

Add command to lint all files and fix one error

46f0fba

Remove unnecessary Pipfile

ce352ec

Add back testlocal recipe

d4e2682

Fix linting task

79b4eef

dhruvkb reviewed Sep 16, 2021

View reviewed changes

.github/workflows/integration-tests.yml Outdated Show resolved Hide resolved

sarayourfriend and others added 3 commits September 16, 2021 13:23

Use working-directory instead of cd

caa41ee

Fix circular import bug introduced during import order correction

ea9524b

Add missing trailing slash

728e4e5

dhruvkb approved these changes Sep 17, 2021

View reviewed changes

dhruvkb reviewed Sep 17, 2021

View reviewed changes

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

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

Use more cautious curl approach

f4c9023

Co-authored-by: Dhruv Bhanushali <dhruv_b@live.com>

sarayourfriend added 3 commits September 17, 2021 07:07

Fix pre-commit-config

c5192a0

Mention just in prereqs for running the repo

45810ef

Add explanations for flake8 ignores

85f125b

Remove duplicated config

0683180

sarayourfriend closed this Sep 21, 2021

sarayourfriend deleted the add/pre-commit branch September 21, 2021 20:52

	- repo: https://gitlab.com/pycqa/flake8
	- repo: https://github.com/PyCQA/flake8

Add just and pre-commit #201

Add just and pre-commit #201

Conversation

sarayourfriend commented Sep 7, 2021

Fixes

Description

Technical details

Tests

Checklist

Developer Certificate of Origin

dhruvkb commented Sep 8, 2021 • edited Loading

sarayourfriend commented Sep 8, 2021

AetherUnbound left a comment

Choose a reason for hiding this comment

krysal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb Sep 14, 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

dhruvkb commented Sep 15, 2021 • edited Loading

sarayourfriend commented Sep 15, 2021 • edited Loading

dhruvkb commented Sep 15, 2021

sarayourfriend commented Sep 16, 2021

dhruvkb left a comment

Choose a reason for hiding this comment

dhruvkb Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb commented Sep 16, 2021

sarayourfriend commented Sep 16, 2021

dhruvkb commented Sep 16, 2021

dhruvkb left a comment • edited Loading

Choose a reason for hiding this comment

dhruvkb 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

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

dhruvkb commented Sep 17, 2021 • edited by zackkrida Loading

dhruvkb left a comment

Choose a reason for hiding this comment

sarayourfriend commented Sep 17, 2021

sarayourfriend commented Sep 17, 2021

zackkrida commented Sep 19, 2021

dhruvkb commented Sep 20, 2021

sarayourfriend commented Sep 21, 2021

dhruvkb commented Sep 8, 2021 •

edited

Loading

sarayourfriend Sep 13, 2021 •

edited

Loading

dhruvkb Sep 14, 2021 •

edited

Loading

dhruvkb commented Sep 15, 2021 •

edited

Loading

sarayourfriend commented Sep 15, 2021 •

edited

Loading

dhruvkb Sep 16, 2021 •

edited

Loading

dhruvkb left a comment •

edited

Loading

dhruvkb commented Sep 17, 2021 •

edited by zackkrida

Loading