Black support for the Python codebase

[arikfr]

What type of PR is this? (check all applicable)

• Other

Description

• Applied Black formatting to all the current codebase. 🏴
• GitHub Action to auto format code committed to master
• Update contribution guide re. code style/black.

Related Tickets & Documents

Closes #3806.

[jezdez]

OMG!!! 🏴

[arikfr]

This PR adds a check to CircleCI that will fail if the code is not formatted with Black. Considering that formatting with Black (and Prettier) can be automated I wonder what we should do:

1. Just add pre-commit that handles this (either to Husky or switch to pre-commit).
2. Have a GitHub action that automatically fixes the code and commits back. The downside is that it might be confusing for the end user and with some external PRs we don't have commit option.
3. Have a GitHub action that fixes the code and makes another PR (to be merged into the original one) with the changes. The downside here is that it can become confusing when there are multiple executions of the check before the PR is merged (will create duplicate PRs, or will have to add logic that handles this).

Any thoughts?

[jezdez]

I would suggest to go with (1) and add a pre-commit config.

Since the CI job is already separate, it's easy to see why a test failed from the GitHub status UI. But maybe rename the job to "black-formatting" as it's more obvious to contributors than just "black"?

[arikfr]

Since the CI job is already separate, it's easy to see why a test failed from the GitHub status UI.

It's relatively easy to understand what job failed. But I'm not sure it's obvious to everyone (specially new people) what this failure means, how to fix it, etc.

Another aspect that it's boring to do things a machine can do :-)

Maybe we can start by adding a comment on the PR if it fails that explains what Black is and how to remedy?

[arikfr]

I've been experimenting with a middle ground solution: ability to trigger GitHub Action to run Black & Prettier with a comment on the PR. The end solution I was aiming for is that the check will post a comment on the PR with a text like:

Your PR doesn't meet our formatting standards. We use Black for auto formatting. You can run black locally (link to instructions) or trigger it by reacting to this comment with 👍.

Then the 👍 reaction would trigger the GitHub Action. My POC used a comment "slash command", but apparently comments run with the context of the master branch. To trigger this on the correct branch/repo there is more work required. I'm posting here my work just in case someone might want to pick it up:

name: Python Code Formatting

on: issue_comment

jobs:
  build:
    runs-on: ubuntu-latest

    container:
      image: python:3.7.4-alpine
    
    steps:
    - name: Check for Command
      id: command
      uses: xt0rted/slash-command-action@v1
      with:
        repo-token: ${{ secrets.GITHUB_TOKEN }}
        command: black
        reaction: "true"
        reaction-type: "+1"
        allow-edits: "false"
        permission-level: none
    - uses: actions/checkout@v1
    - name: Install Black
      run: apk add gcc musl-dev && pip install black
    - name: Run Black
      run: black redash
    - name: Commit changes # This is the step name that will be displayed in your runs
      uses: EndBug/add-and-commit@v2.1.0 # You can change this to use a specific version
      with: # See more info about inputs below
        author_name: Black
        author_email: team@redash.io
        message: "Autoformatted code with Black"
        path: "redash"
        pattern: "*.py"
        force: false
      env:
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

The idea is to infer the Pull Request from the GitHub event data (like this action) and then use the Pull Request number to get the branch/repo info using GitHub's API.

[arikfr]

Another option: run the auto formatter when merging into master?

[arikfr]

I changed the implementation here a bit: instead of checking whether the code is "compliant" with Black's formatting, I added a GitHub Action to format any code committed to master.

Considering this is an automatic step (and hopefully non destructive) it felt counter productive to block Pull Requests based on whether they are formatted with Black or not.

I will apply the same logic to Prettier integration in another Pull Request.

[jezdez]

Interesting idea! Could I interest you in also auto applying isort? There is a black-compatible config here: https://black.readthedocs.io/en/stable/the_black_code_style.html#how-black-wraps-lines

[arikfr]

Interesting idea! Could I interest you in also auto applying isort?

The problem with applying isort is that unlike Black it might change actual implementation in case some imports have side effects and their order is important.

While side effects on module import is a bad idea, it does happen. I don't remember if the main codebase has them (I hope not, but...) but I know for sure that we have some at the moment in the SaaS code base to deal with Python 2/3 Pickle compatibility and some other things related to running Celery/RQ and Python 2/3 codebases in a single environment.

So while I'm not opposed to isort, I'm not comfortable with applying it automatically when merging too master.

[jezdez]

@arikfr Makes total sense, we've seen import time side effects as well, when we worked on our extensions.

Maybe instead we could apply isort carefully once and exclude the delicate import statements with explicit skip comments? Then we could at least run an isort check on pull requests via a GH action?

[arikfr]

Ok, the idea to apply on merge to master is a bust:
https://github.com/getredash/redash/runs/343728757

master is a protected branch and you can't commit to it directly. 🤔

[jezdez]

Ah, d'oh! 🤦🏻‍♂️

[jezdez]

@arikfr IIRC you can restrict who can push to master, maybe since you're using an API token for this, it can be applied to only being you?

[jezdez]

[arikfr]

It won't help, because the required status check protection will fail the push. Also, I don't think we control with whose token it runs.

The two options I thought of:

Two options now:

1. Remove the automatic actions and add a check step to make sure the code is formatted with Black/Prettier when a PR is open. This means that author will need to figure how to apply black/prettier before the PR can be checked or merged.
2. Remove branch protection.

[jezdez]

In my experience new contributors often struggle with understanding the usefulness of code formatters and can be quite frustrated by checks failing because of code formatting. So anything automated or easy to run black would make sense the most. E.g. running a single command locally to apply the code formatting.

For practiced developers this may not be a huge barrier.

There is still the black_out bot, that we could run ourselves: https://github.com/Mariatta/black_out

OTOH since I've seen many projects adopting it, maybe using pre-commit for black and prettier would make sense? There are a few people investigating ways to use it for bots as well: pre-commit/pre-commit.com#211 E.g. https://restyled.io/?

[arikfr]

@jezdez thanks! I will try restyled.io. I think it will do what we need with minimum effort for all involved.

[arikfr]

Restyled: 0a2dd83.

[jezdez]

@arikfr Huzzah!