Page MenuHomeSoftware Heritage

pre-commit: Add isort hook and configuration
ClosedPublic

Authored by anlambert on Sep 17 2020, 11:20 AM.

Details

Summary

Run isort when committing by adding a new pre-commit hook.

Use the recommended isort configuration in order to use it with black
without any conflicts (https://black.readthedocs.io/en/stable/compatible_configs.html#isort).

Diff Detail

Repository
rDWAPPS Web applications
Branch
pre-commit-isort
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15214
Build 23457: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 23456: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D3972 (id=13998)

Could not rebase; Attempt merge onto cec8f16fe0...

Updating cec8f16f..c0583f89
Fast-forward
 .pre-commit-config.yaml | 11 ++++++++++-
 pyproject.toml          |  8 ++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)
Changes applied before test
commit c0583f89ce168c121471cd8deac059be1c6201d9
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Sep 17 11:17:16 2020 +0200

    pre-commit: Add isort hook and configuration

commit 91569bf6b094a88d7f4557dc47332b3917ef8394
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Sep 17 11:10:06 2020 +0200

    pre-commit: Update flake8 hook configuration
    
    flake8 hook has been removed from https://github.com/pre-commit/pre-commit-hooks
    so now use the one from https://gitlab.com/pycqa/flake8

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/373/ for more details.

Definitely yes.

(bikeshedding incoming)

I'm wondering whether we should add a specific section for django imports. Something like the following:

known_django = 'django,rest_framework'
sections = 'FUTURE,STDLIB,THIRDPARTY,DJANGO,FIRSTPARTY,LOCALFOLDER'

Which generates (swh/web/common/utils.py):

# Copyright (C) 2017-2020  The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information

from datetime import datetime, timezone
import re
from typing import Any, Dict, Optional

from bs4 import BeautifulSoup
from docutils.core import publish_parts
import docutils.parsers.rst
import docutils.utils
from docutils.writers.html5_polyglot import HTMLTranslator, Writer
from iso8601 import ParseError, parse_date
from prometheus_client.registry import CollectorRegistry

from django.http import HttpRequest, QueryDict
from django.urls import reverse as django_reverse
from rest_framework.authentication import SessionAuthentication

from swh.web.common.exc import BadInputExc
from swh.web.common.typing import QueryParameters
from swh.web.config import get_config

instead of

# Copyright (C) 2017-2020  The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information

from datetime import datetime, timezone
import re
from typing import Any, Dict, Optional

from bs4 import BeautifulSoup
from django.http import HttpRequest, QueryDict
from django.urls import reverse as django_reverse
from docutils.core import publish_parts
import docutils.parsers.rst
import docutils.utils
from docutils.writers.html5_polyglot import HTMLTranslator, Writer
from iso8601 import ParseError, parse_date
from prometheus_client.registry import CollectorRegistry
from rest_framework.authentication import SessionAuthentication

from swh.web.common.exc import BadInputExc
from swh.web.common.typing import QueryParameters
from swh.web.config import get_config

I also suggest turning on force_sort_within_sections = true (which sorts from x import y before import z instead of grouping from imports together at the bottom)

I'm wondering whether we should add a specific section for django imports.

Nice ! I think we should do it, this makes sense regarding the numerous django imports in swh-web source files.

I also suggest turning on force_sort_within_sections = true

Ack, I will update that diff accordingly.

Build is green

Patch application report for D3972 (id=14003)

Could not rebase; Attempt merge onto cec8f16fe0...

Updating cec8f16f..53e1a306
Fast-forward
 .pre-commit-config.yaml | 11 ++++++++++-
 pyproject.toml          | 12 ++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
Changes applied before test
commit 53e1a306f75f9d330c9716030464de7531e1d2d9
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Sep 17 11:17:16 2020 +0200

    pre-commit: Add isort hook and configuration

commit 91569bf6b094a88d7f4557dc47332b3917ef8394
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Sep 17 11:10:06 2020 +0200

    pre-commit: Update flake8 hook configuration
    
    flake8 hook has been removed from https://github.com/pre-commit/pre-commit-hooks
    so now use the one from https://gitlab.com/pycqa/flake8

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/374/ for more details.

I'm okay with this change. But I think other team members should have a stab at the color of the bikeshed too.

In D3972#98074, @olasd wrote:

I'm okay with this change. But I think other team members should have a stab at the color of the bikeshed too.

Ack, I will wait before landing this then.

In D3972#98074, @olasd wrote:

I'm okay with this change. But I think other team members should have a stab at the color of the bikeshed too.

Ack, I will wait before landing this then.

Maybe this deserves a discussion on a dedicated task or at least a heads-up in a swh-devel@ mail.

Maybe this deserves a discussion on a dedicated task or at least a heads-up in a swh-devel@ mail.

I have created T2610 on the subject.

In D3972#98074, @olasd wrote:

other team members should have a stab at the color of the bikeshed too.

Well, Black, of course!

LGTM, it matches the style I already use (except the django thing, but it makes sense to me)

Also fine with me, thanks.

Note: I think that supersedes D3519 now (providing this is done everywhere).
For doing it everywhere @vlorentz added a command line tool to automate this
change across multiple repos in swh-environment/bin/change-all-repos btw ;)

This revision is now accepted and ready to land.Sep 17 2020, 3:48 PM

Also fine with me, thanks.

Note: I think that supersedes D3519 now (providing this is done everywhere).
For doing it everywhere @vlorentz added a command line tool to automate this
change across multiple repos in swh-environment/bin/change-all-repos btw ;)

Yes, I used it for the flake8 pre-commit hook update. Cool stuff !

Regarding D3519, I guess I can Commandeer Revision on it once isort configuration pushed to repos and abandon it afterwards ?

Regarding D3519, I guess I can Commandeer Revision on it once isort configuration pushed to repos and abandon it afterwards ?

Yes, sounds reasonable to me, referencing this task and discussion along the way in that other diff ;)

anlambert edited the summary of this revision. (Show Details)

Update: Global isort configuration and processing have been performed on all repos (T2610), modify that diff to add dedicated section for django imports

Build is green

Patch application report for D3972 (id=14032)

Rebasing onto a35c45e862...

Current branch diff-target is up to date.
Changes applied before test
commit 626b1e5b75ded03ba310395b712b0ccac26cbd40
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Sep 17 18:15:03 2020 +0200

    isort: Put django imports in a dedicated section

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/375/ for more details.