Paths

Table of Contentst

Differential D3972

pre-commit: Add isort hook and configuration
ClosedPublic
Actions

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

Details

Reviewers

ardumont

Group Reviewers

Reviewers

Commits

rDWAPPS626b1e5b75de: isort: Put django imports in a dedicated section

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

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 15301
Build 23564: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 23563: arc lint + arc unit

Event Timeline

anlambert created this revision.Sep 17 2020, 11:20 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptSep 17 2020, 11:20 AM

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.

Harbormaster completed remote builds in B15209: Diff 13998.Sep 17 2020, 11:27 AM

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.

Update: Address @olasd comments

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.

Harbormaster completed remote builds in B15214: Diff 14003.Sep 17 2020, 1:21 PM

anlambert edited the summary of this revision. (Show Details)Sep 17 2020, 2:03 PM

anlambert removed a parent revision: D3971: pre-commit: Update flake8 hook configuration.

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#98084, @anlambert wrote:

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 ;)

ardumont accepted this revision as: ardumont.Sep 17 2020, 3:48 PM

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

In D3972#98165, @ardumont wrote:

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 ;)

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.

Harbormaster completed remote builds in B15301: Diff 14032.Sep 17 2020, 6:24 PM

Closed by commit rDWAPPS626b1e5b75de: isort: Put django imports in a dedicated section (authored by anlambert). · Explain WhySep 17 2020, 6:25 PM

This revision was automatically updated to reflect the committed changes.

anlambert added a commit: rDWAPPS626b1e5b75de: isort: Put django imports in a dedicated section.