Support auto-formatting

[sacOO7]

• Current flake8 doesn't support auto-formatting.
• Add black tool available in the market to support autoformatting.
• Configure it to be compatible with flake8.
• https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html
• This is also needed to support auto-indenting after source code is generated using unasync.py.
• Currently, it takes time to fix linting errors and add explicit support for auto-indent in unasync.py
• There are lots of dummy commits being added as a part of fixing linting issues : ( still, properly linted code is not guaranteed.
• A mix of single/double quotes can be found throughout the code with unaligned indentation for function/method definitions.

┆Issue is synchronized with this Jira Task by Unito

[sync-by-unito[bot]]

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3897

[owenpearson]

I personally have mixed feelings about this - I use black locally for development but I'm not a fan of some of the behaviour, for example:

# before black
rename_classes = [
    "AblyRest",
    "Push",
    "PushAdmin",
    "Channel",
    "Channels",
    "Auth",
    "Http",
    "PaginatedResult",
    "HttpPaginatedResponse"
]

# after black, much less readable
rename_classes = [ "AblyRest", "Push", "PushAdmin", "Channel", "Channels",
    "Auth", "Http", "PaginatedResult", "HttpPaginatedResponse" ]

Personally my recommendation is to just use range formatting locally when you need it, but not to automatically format every single file. For dummy commits, i don't think i've ever pushed a 'fix formatting' commit to this repo, it's very easy to amend previous commits using git rebase if you forgot to run flake8.

[sacOO7]

I think we should be able to configure some of those stylings using black or other formatter (brunette). The main issue is that everyone can have their own perspective about styling, like currently we have a mix of single and double quotes. Black enforces to use of double quotes to make sure there is consistent styling across the codebase. Also, without formatter, it does take a good amount of time to look at failing lint warnings and then fix them. I am not sure how rebase can work in this case. Are we still saying to add one more commit for fixing linting? The only option I can see without an additional commit is updating the existing commit and then git push -f. Also, force push is not always a good idea just for the sake of linting. Some of the devs even avoid force push. As I mentioned, this is not the only issue. The problem is consistent styling across the codebase. If we have a formatter configured to do the same, it should solve all of those problems.

[sacOO7]

Also, I am sure when we will have new devs onboarded to work on ably-python in the future, there's gonna be a discussion again to add a formatter for consistent styling : (

[sacOO7]

@owenpearson it seems the issue you mentioned has already been raised https://github.com/psf/black/issues/601

[sacOO7]

Also, it seems we can disable behavior for the short list using -> https://github.com/psf/black/issues/650#issuecomment-463790649. It should work fine for a long list without disabling, like the one you mentioned.

[owenpearson]

Ah fair enough, i didn't realise you could configure that behaviour with black, that's good to know :)

I do certainly see the value in having consistent formatting, I just think there are tradeoffs because auto formatters are never perfect. I'm not saying we shouldn't add one at all, just that personally I'm happy without it 🤷

As for force pushing, I would definitely recommend using force push to fix linting/formatting issues rather than pushing separate commits - ideally each commit on the main branch should represent a valid state of the project (ie all CI checks passing). I do this all the time and it's never caused any issues, the worst that can happen is that someone else has the branch checked out and they have to git reset --hard origin/branch_name. Worth mentioning that even with an autoformatter script this will still be an issue because developers aren't gonna remember to run it before every commit.

[sacOO7]

That's why most people use pre-commit hook to run formatting/linting process.

[sacOO7]

https://pypi.org/project/pre-commit-hooks/ seems like a good option

[owenpearson]

i dunno if "most people" is correct, personally i don't like them. and like before, even if we add all of this tooling there will still be occasions where you push a commit which leaves the repo in an invalid state so it's not a comprehensive alternative to rebase + force push

[sacOO7]

I know. I am not a fan of git hooks either : ( I think once devs know, that linting is a mandatory check on CI, they will eventually get used to it. At least, having a tool is better than nothing, saves a lot of manual work : )