Introduce formatter

[keisuke-umezawa]

Motivation

If we introduce a formatter to Optuna and just use it, we do not need to think about formatting.

Description

There are famous formatters in python:

1. autopep8 (current)
2. Black
   • example of applying black [DO NOT MERGE] Apply black #957
3. yapf

c.f. https://blog.frank-mich.com/python-code-formatters-comparison-black-autopep8-and-yapf/

We need to discuss what formatter is best.

Differences of above formatters

black

• Use " instead of '.
• Add trailing commas to expressions that are split by comma where each element is on its own line.
• Break a line before a binary operator when splitting a block of code over multiple lines.

black vs yapf

c.f. https://news.ycombinator.com/item?id=17155048

• YAPF would at times not produce deterministic formatting (formatting the same file the second time with no changes in between would create a different formatting); Black treats this as a bug;
• YAPF would not format all files that use the latest Python 3.6 features (we have a lot of f-strings, there's cases of async generators, complex unpacking in collections and function calls, and so on); Black solves that;
• YAPF is based on a sophisticated algorithm that unwinds the line and applies "penalty points" for things that the user configured they don't like to see. With a bit of dynamic programming magic it arrives at a formatting with the minimal penalty value. This works fine most of the time. When it doesn't, and surprised people ask you to explain, you don't really know why. You might be able to suggest changing the penalty point value of a particular decision from, say, 47 to 48. It might help with this particular situation... but break five others in different places of the codebase.

[keisuke-umezawa]

imo, I want to use black as our formatter.

[c-bata]

+1 to use black.

I also use black at https://github.com/CyberAgent/cmaes.

[kidrahahjo]

Flake8 can also be a good formatter at times.
It's not rigid and also it doesn't automatically fix the errors which is often useful.

[hvy]

Leaving some community statistics that shows how Black is being more adopted.

Stars: https://star-history.t9t.io/#google/yapf&psf/black
Used by: Black has twice as many.

[keisuke-umezawa]

@/here
How can I archive conclusion? 🤔

btw, I will create PR with black.

[hvy]

I think most of the core members are okay with black, so far. A PR could be a starting point to discuss it more in detail. Just curious how it will be different from #957 ?

[c-bata]

In my opinion, we need to avoid conflict with other pull requests as possible as we can. At least, #957 significantly increases the costs of other pull requests.

[keisuke-umezawa]

I created two PRs for applying black to all files with following conditions, because it is better to compare them. Here, string format means converting ' to ".

• w/ string format Apply black to all files with string normalization. #987
  • pros
    • we do not need to think about ' and " anymore, because they are automatically converted to ".
  • cons
    • More file changes
    • We can not use '.
• w/o string format Apply black to all files without string normalization. #988

[hvy]

Thank you for the different PRs. The actual changes are much easier to see if the string normalizations are separated (as they are different commits). Briefly discussing this with the team, it seems like most are okay with introducing black, and in particular to align to " for strings. One of the purposes of the formatter is to keep a consistent style not having the developer have to think about it. It's of course open for discussion but I think we can proceed with strings being normalized.

However, as pointed out in #956 (comment), this might increase the cost of ongoing PRs. Do you have any suggestion on how to proceed? As most PRs are local enough, I for instance think that splitting up the PR won't benefit that much. We could try to merge larger PRs or long-running PRs prior to introducing the formatter.

[keisuke-umezawa]

However, as pointed out in #956 (comment), this might increase the cost of ongoing PRs. Do you have any suggestion on how to proceed? As most PRs are local enough, I for instance think that splitting up the PR won't benefit that much. We could try to merge larger PRs or long-running PRs prior to introducing the formatter.

That is true. If we want to apply black, we need to think how to do it. Maybe, one of the best chances is just before next release? 🤔

[keisuke-umezawa]

@hvy I opened a new PR to merge changes. #1020

As we discussed, we will apply black as follows:

1. Apply black with string normalization and 99 chars in a line.
2. Enable black instead of autopep8.
3. Check black in circleci.

[hvy]

I created a PR addressing above comment #1030 (c.f. #1020 (comment)). It has now been merged so let me close this issue. Thanks a lot @keisuke-umezawa for the long running investigation.