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
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10933
Build 16448: tox-on-jenkinsJenkins
Build 16447: arc lint + arc unit

Event Timeline

vlorentz created this revision.Mar 5 2020, 3:04 PM
vlorentz updated this revision to Diff 9865.Mar 5 2020, 3:08 PM

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

olasd added a subscriber: olasd.Mar 5 2020, 3:41 PM

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
29

Should probably be in the pyproject.toml?

zack added a subscriber: zack.Mar 5 2020, 3:45 PM

+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 !

olasd added a comment.EditedMar 5 2020, 3:53 PM
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.

zack added a comment.EditedMar 5 2020, 4:00 PM
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")
vlorentz updated this revision to Diff 9894.Mar 6 2020, 3:09 PM

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)
olasd added a comment.Mar 10 2020, 8:06 PM
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.

vlorentz updated this revision to Diff 10085.Mar 16 2020, 4:17 PM

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)
vlorentz updated this revision to Diff 10088.Mar 16 2020, 5:06 PM

fix test (same one as before).