Introducing Black and Pylint

[m-czernek]

Summary

In this discussion, I am proposing the following:

• Use black to format contributions.
• Use pylint to check contributions.
• Adopt the Google Style Guide for Python (pylintrc included)
  • notable change: Modify the 80 characters per line rule to 88 characters per line to align ourselves with Black's default 88 characters per line.

If accepted, I will do the following:

• Format select Python files in the uyuni repository.
• Work around all current pylint issues to bring pylint reports 10/10 score.
• Provide standardized environment for executing pylint and black.

After that, we can implement automated checks on every PR, e.g. by using Github Actions or Jenkins, and enforce that contributors use unified formatting rules and adhere to the pylint static analysis.

Proposal

Currently, different files use different or no standards, contain unused code (e.g. imports), and multiple formatting styles.

The proposed solution is to:

1. Format Python files with black.
2. Workaround current pylint issues by adding a # pylint: disable=... comment to all lines that currently do not adhere to the pylint standard.
3. Configure checks in Github Actions to execute black and pylint on every PR.
4. Provide documentation on how to execute black and pylint locally, either by using local environment or a container.

Consequently:

• Developers do not have to think about formatting.
• All files report a perfect 10/10 pylint score.
• New changes can only lower the score, which means that our automated checks run implicitly on new code only.
• Developers are expected to fix old code that does not adhere to the linting standards incrementally.

Implementation

Black and Pylint

Formatting is automatic, consisting running black on all files that we want to modify.

Disabling pylint for currently non-conforming lines is automated by custom scripts. I propose automating this task. You can see my proof-of-concept script at https://github.com/m-czernek/autolint/blob/main/autolint.py.

Though the scripts still require manual checking, we can automate the majority of the effort.

Providing Authoritative Execution Environment

I propose creating a container image that includes black, pylint, and its configuration files.
Contributors can either create local environment, or use the container image in case they cannot use our local environment (e.g. due to not using the latest Python version).

I propose a new container instead of reusing our testing container to:

1. Reduce the container footprint.
2. Use the latest Python/pylint/black versions.

You can view a proof-of-concept container at https://github.com/m-czernek/autolint/blob/main/Containerfile.

Discussion

How do we ensure that the output of Pylint and Black is compatible with our currently supported Python version, i.e. Python 3.6?

The black CLI enables us to select the py36 version to ensure compatibility with Python 3.6. Similarly, Pylint enables us to use the py-version flag to also ensure compatibility with Python 3.6.

We will migrate the options when we move to the newer supported Python versions.

Different users might use different OSes and different Python versions. How do we deal with that?

We provide a containerized environment that produces the authoritative result.

What files do we want to format and check?

Any non-test Python files in the /python directory. We can modify other files as well, e.g. Python files in /spacewalk or Python tests, if the team finds this desirable.

On what branches do we want to enforce the changes?

The main branch (master). However, due to the fact that the effort is mostly automated, we can implement the changes on any branches if the team finds this desirable.

Links

• Previous pylint-related discussion
• Ongoing example PR to see what the formatted files would look like
• Proof-of-concept scripts for automating the effort
• Google's Python styleguide
• Black code formatter
• Pylint static code analyser

[m-czernek]

CC @meaksh @vzhestkov @juliogonzalez @mcalmer @agraul @renner ; feel free to ping others as well of course.

Edit: I see I forgot to ping @ycedres , my apologies, I copy-pasted the pings from the previous discussion.

[juliogonzalez]

I like the idea, but some questions and thoughts:

Providing Authoritative Execution Environment

For the container, it would be ideal if we could have the image definition at OBS, as we do for other containers for CI (https://build.opensuse.org/project/show/systemsmanagement:Uyuni:Master:Docker, and similar stuff for SUSE Manager), but I am not sure how hard it will be as your Dockerfile uses pip, and OBS doesn't allow access to the internet during building time (there could be some workarounds, however, such as downloading the requirements with a service).

How do we ensure that the output of Pylint and Black is compatible with our currently supported Python version, i.e. Python 3.6?

We need to define what will be the minimum supported version. Ion should know. I hope we don't have any python2 retro compatible code...

On what branches do we want to enforce the changes?

How black will work? Autoformatting the PRs I guess? Or autoformatting branches after PRs are merged?

Depending on where we want to run this, there are different things to consider.

[m-czernek]

Thank you Julio!

For the container, it would be ideal if we could have the image definition at OBS

Yep, I'm now working on building the container in OBS, this should not be a problem hopefully :)

We need to define what will be the minimum supported version.

Excellent point, I assumed we only target Python 3.6. I'll let others chime in if this assumption is incorrect :)

How black will work? Autoformatting the PRs I guess? Or autoformatting branches after PRs are merged?

As part of this effort, we would re-format all current files that we want to enforce the changes on and commit that to master.

The automated check then runs black against the files in your PR and if the files change, the check fails. This means that the developer didn't run black on the files and should do so.

This way, the developer has control over applying the formatting changes (which might be meaningful, e.g. when applying pylint ignore comments) and can test that everything works as expected.

[juliogonzalez]

As part of this effort, we would re-format all current files that we want to enforce the changes on and commit that to master.

Looks OK, as long as we do the re-format with a PR. We could block the master branch if needed.

@uyuni-project/release-engineering is there anything I could be missing here?

[mcalmer]

As long as it is easy usable I am fine. But I just want to execute 1 command and installation of the environment should be also easy doable.

[juliogonzalez]

For using the container easily, we could just have a simple wrapper that starts an image, passed the current repository as a bind mount, runs black and/or pylint and reports the results. It could even do the changes for you, as with podman you can easily map the permissions, as done at https://github.com/uyuni-project/uyuni-docs-helper (yes, I know this wrapper has more options, but it's because of the required use cases)

[agraul]

I like the approach that's described in the post! There are a few details that I'd like to comment on.

1. Authoritative Execution Environment
   This is a must-have since we developers use different operating systems and we all need to get the same results. Likewise for CI environments. In the "Implementation" section you mention that the configuration files will be part of the container image while a local environment is also an option. Is it beneficial to have the configuration files inside the container opposed to leave them e.g. at the root of the git repository and use them from the container?

I agree with others that we should include a script that runs the container transparently. A while ago I created a PoC that has some issues passing multiple args but generally works:

#!/bin/sh
IMAGE=${IMAGE:-systemsmanagement/uyuni/master/docker/containers/uyuni-master-pgsql}
REGISTRY=${REGISTRY:-registry.opensuse.org}
GITROOT=$(git rev-parse --show-toplevel)
REL_HERE=$(git rev-parse --show-prefix)
ENGINE=${CONTAINER_ENGINE:-podman}

echo "$ENGINE" run -it --rm -v "$GITROOT":/code "$REGISTRY/$IMAGE" /bin/sh -c "cd /code/$REL_HERE && pylint --rcfile /code/pylintrc $@"
"$ENGINE" run -it --rm -v "$GITROOT":/code "$REGISTRY/$IMAGE" /bin/sh -c "cd /code/$REL_HERE && pylint --rcfile /code/pylintrc $@"

2. About What files do we want to format and check?
   Anything that @uyuni-project/python owns should be covered in my opinion. I'd also include tests, they are as important as product code.

3. On what branches do we want to enforce the changes?
   I agree with starting with master. Backporting formatting changes to 4.3 is an option we can use if backporting bug fixes becomes troublesome. Back when Salt first started re-formatting their code we didn't re-format our branch and backporting was still pretty manageable.

4. So far this proposal does not include isort. Is this something we want to add? It's a tool that sorts and groups imports (in a black compatible way with --profile black) and is also used by e.g. Salt. We could also add it later, but maybe it's a good idea to include it in the "re-format project PR"

5. Something not mentioned in this proposal that I want to highlight is that black is pretty safe. It was used to reformat big code bases e.g. Instagram in one go, I'm pretty sure it's good enough for us as well. black checks that the AST before and after formatting the code is semantically equivalent. We'll still review the changes but I trust it more than other tools.

[ycedres]

So I understand that as a first step we would need to run black on the selected files/directories/repositories. Which steps would we follow here? As @agraul mentioned it would be interesting to cover everything owned by @uyuni-project/python but I'm not sure what would be the criteria. Should we do this incrementally?

[agraul]

I would do it in one sweep, that way we only have one PR that takes care of formatting and we can add the commit(s) to https://github.com/uyuni-project/uyuni/blob/master/.git-blame-ignore-revs