Page MenuHomeSoftware Heritage

Enable Black on swh-core
AbandonedPublic

Authored by vlorentz on Mar 5 2020, 3:04 PM.

Details

Reviewers
None
Group Reviewers
Reviewers
Summary

We talked about this a couple of times already. I think it's a good idea
to uniformize the style like this.

To keep this diff readable, I disabled string normalization (which turns
single-quotes into double-quotes). We should probably enable it in the end,
but I don't have a strong opinion on it.

Black's reasoning is that English text is more likely to use apostrophes
than quotes (which I think is true for our code as well).

What do you think?

Diff Detail

Repository
rDCORE Foundations and core functionalities
Branch
black
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11157
Build 16842: tox-on-jenkinsJenkins
Build 16841: arc lint + arc unit

Event Timeline

fix test (it depends on line numbers...)

I'm all for this change. This should get rid of a few # noqas as well.

Should we use the opportunity to enable isort?

As a side note, I've looked at whether flake8 supports pyproject.toml and the upstream discussion about it is a sight to behold: https://gitlab.com/pycqa/flake8/issues/428; but it seems that the main concern about the addition of pyproject.toml (that it makes pip enable isolated builds) shouldn't impact us as we only need setuptools for running setup.py these days.

.pre-commit-config.yaml
28

Should probably be in the pyproject.toml?

+1 on the general idea.

About this specific point:

To keep this diff readable, I disabled string normalization (which turns single-quotes into double-quotes). We should probably enable it in the end, but I don't have a strong opinion on it.

I don't see much value in doing large, annoying, purely syntactic migration in multiple steps. Just go ahead and use the default opinionated formatting in the first (and hopefully only) migration to Black.
(And I say this even if I personally prefer single quotes. But, meh, if the opinionated default is double quotes, let's just go for it. It'll be better than the inconsistencies we already have.)

A couple of more TODOs that should be IMO implemented as part of this:

Thanks for pushing this forward !

In D2772#66234, @zack wrote:

About this specific point:

To keep this diff readable, I disabled string normalization (which turns single-quotes into double-quotes). We should probably enable it in the end, but I don't have a strong opinion on it.

I don't see much value in doing large, annoying, purely syntactic migration in multiple steps. Just go ahead and use the default opinionated formatting in the first (and hopefully only) migration to Black.
(And I say this even if I personally prefer single quotes. But, meh, if the opinionated default is double quotes, let's just go for it. It'll be better than the inconsistencies we already have.)

I think this was purely a diff readability helper (as our default is single quotes, it'd have changed almost every line to switch it). I agree that we should switch on string normalization as well.

In D2772#66241, @olasd wrote:

I think this was purely a diff readability helper (as our default is single quotes, it'd have changed almost every line to switch it). I agree that we should switch on string normalization as well.

Yeah, I get that. I guess my point is twofold here:

  1. if double quote is winning anyway (as default/popular in the python world), then we want to switch to it too
  2. so if we are going to switch to that too, let's do it now (my take would be that these diffs will not be readable/meaningful anyway, they will just be accompanied by commits saying "apply Black, no other changes")

move all of black config to pyproject.toml

+1 for the idea and starting implementing this ;)

We'll wait to move this forward until @anlambert has a chance to chime in. @douardda reported being "meh" about it (but not vetoing the change)

Various bikesheds have been painted here and on IRC, and the consensus so far seems to be:

  • we should move forward with the change
  • we will enable string normalization towards double quotes
  • we will use the default black line length (88 chars), even though that means people having to configure their text editor for a non-default line wrapping value (if they use line wrapping at all)
In D2772#67164, @olasd wrote:
  • we will use the default black line length (88 chars), even though that means people having to configure their text editor for a non-default line wrapping value (if they use line wrapping at all)

Ah, I forgot to mention: looks like a few implementations of the EditorConfig spec support setting max_line_length, even though the field hasn't been standardized (for 7 years); maybe that's something we should consider:
https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length

(but before introducing another top-level config file, I'd like us to find a way to manage these kinds of environment-wide changes better than adding subtly non-identical files manually in all repos by hand)

In D2772#67168, @olasd wrote:

(but before introducing another top-level config file, I'd like us to find a way to manage these kinds of environment-wide changes better than adding subtly non-identical files manually in all repos by hand)

Unlike most other config files, .editorconfig would work if we put it in swh-environment.

We'll wait to move this forward until @anlambert has a chance to chime in.

I do not have any concerns on using black in our codebase, let's move forward with this change.

enable string normalization

vlorentz retitled this revision from RFC: Enable Black on swh-core to Enable Black on swh-core.Mar 16 2020, 4:17 PM
vlorentz edited the summary of this revision. (Show Details)

fix test (same one as before).

Landed as b980a2e88d2100fb0284cb33d566ad62b47ee16a and cdd8268f9f0320452b445a09a85de89e1fc86090.