340-automatic-code-formatting

[wyli]

Fixes #340.

Description

• initial automatic formatting with black
• manual fixes of an issue after the auto formatting
• versioneer files are included in the formatting, as we might modify it in the future
• Jupyter notebooks excluded (shall we move the notebooks to another repo, it's very difficult to maintain the content)

this ('opinionated') coding style change affects many areas, would like to get more reviews for this PR @Nic-Ma @ericspod @atbenmurray @hjmjohnson

Status

READY

Types of changes

• Bug fix (non-breaking change which fixes an issue)

[wyli]

/black
FYI the actual bot commands are here https://github.com/Project-MONAI/monai-code-formatter/blob/master/.github/workflows/format.yml#L35-L39

[hjmjohnson]

This looks good overall. A few comments for making development easier for external contributors:

1. Add a file like https://github.com/psf/black/blob/master/pyproject.toml to the main directory so that contributors only need to run black without any command-line options.

# These are implicit command line arguments for black for the MONAI project
[tool.black]
line-length = 120 
target-version = ['py36', 'py37', 'py38']
include = '\.pyi?$'
exclude = '''
/(
    \.eggs
  | \.git
  |  notebooks
  | \.tox
  | \.venv
  | _build
  | buck-out
  | build
  | dist
  )/
'''

2. Consider shorter line lengths. 88 character line length is the optimal point of experiments of millions of lines of code being reformatted with black. Developers with vision or reading difficulties struggle with line lengths greater than 100 characters. There is a good reason why the journal articles, newspapers, magazines use two-column formatting for their content. It is easier to read and comprehend.

[ericspod]

The config file is a good addition. To format the notebooks there is a Black plugin for Jupyter we could consider adding to do this when the notebooks are run. We had discussed the notebooks being run as part of the testing steps as well, the jupyter command can be used to run the cells of a notebook without the interface.

As for line length I really think 120 is the reasonable length. 80 or 88 makes it hard to get a reasonable amount of code on screen, I find it seriously impacts comprehension when reading narrow vs. wide formatted code, especially with the line break needed to fit lines that aren't really that complex. I'm seeing less and less Python code formatted to 80/88 characters as well so the trend appears away from this. Jupyter, Github, and other interfaces seem to display code in a space more optimized for 120 or so characters rather than 80.

[hjmjohnson]

80 or 88 makes it hard to get a reasonable amount of code on screen,

I am fine with 120 characters. I was just pointing out that for a visually impaired person, 120 characters is not "reasonable", and that much research identified that "long lines" are harder to comprehend. I realize that much of the research is dedicated to newsprint, which has different dynamics than code.

Again, I am fine with 120 characters. I do think it needs to be explicitly listed.

[hjmjohnson]

LGTM! Thank you for pushing this through.

[wyli]

/black
thanks for your initial ticket @hjmjohnson it's good to enforce the style as early as possible.
the line length is also aligned with pytorch -- our flake8 config https://github.com/Project-MONAI/MONAI/blob/master/setup.cfg#L24-L38 is mostly copied from https://github.com/pytorch/pytorch/blob/master/.flake8

[Nic-Ma]

This PR is very huge but it seems like the expected formatting output.
Looks good to me.
Do you guys think we can skip this formatting process if PR doesn't have a format issue?
Or just apply it to all PRs, if so, maybe we should not allow it optional by "\black" keyword?
Thanks.

[wyli]

This PR is very huge but it seems like the expected formatting output.
Looks good to me.
Do you guys think we can skip this formatting process if PR doesn't have a format issue?
Or just apply it to all PRs, if so, maybe we should not allow it optional by "\black" keyword?
Thanks.

I think for now this auto formatting is an 'experimental' feature for the codebase. In the future we may drop it if It complicates our code review/merging process. or we may tweak it according to our code collaboration process. so I agree with your first option that we skip it if flake8 doesn't complain.