Set up style guide and tooling for consistency in backend code

[silentninja]

Problem

Right now the backend code has inconsistent styling in few places though they are all valid according to flake8. We should aim to have a style guide that should be followed as much as possible and convert the existing codebase to follow that styleguide

Proposed solution

Come up with a draft of style guide + associated tooling (linters, formatters, etc.), then discuss with the rest of the backend team and finalize it.

Additional context

• This issue originated in a discussion on Matrix.
• GitHub Discussion opened by @silentninja. post any ideas you have here.

[dmos62]

Part of this is a copy-paste of a comment I left in Matrix.

The issue is called "Backend Codebase should follow consistent coding style", but I think that's too strong of a statement. I was expecting to discuss adding automatic formatting rules.

Style is not a very well defined term. I'd say we should discuss the formatting we use and/or want to enforce.

[silentninja]

We definitely cannot expect 100% consistent codebase, but we can add a few rules that would choose one style over the other when given two sensible options

[mathemancer]

As I argued on Matrix: I think the easiest way to get this off the ground quickly would be with the black. I recognize that it's overly opinionated, and we may disagree with some of its formatting (I know I do). I also remember that these reasons are why we removed it from the pipeline initially. However, black is also completely hands-off. We just run it, commit it, and go on by having it run on files in the pipeline. This could be done by the end of the day, without any configuring or thinking needed other than just adding the GH action. There's value in that.

[dmos62]

I wish we could have personal setups and a single neutral rule-set for the repository. We apply black to what we check out to personalize the code, then we apply black to de-personalize when commiting. Not sure if that's feasible.

Totally tangential, but I feel like code versioning systems should account for the fact that there's a lot of changes you can make to code that only affects its formatting and are totally irrelevant to compilers. Then you could format stuff however you like and not affect any other developer's experience. Though, you'd still have to take care to write good code. Short functions and all that.

[kgodey]

@mathemancer and I discussed the usage of black during our check in call today. I don't think the issues we've been facing with code reviews are because of formatting, it's because of different views on code structure and readability. It's better to address those issues as they come up and creating a style guide and using as much linting as we can to enforce the style guide. This will probably involve making smaller pull requests than we're currently making.

The issue with black or any auto-formatting is not the formatting that it produces, it's that it automatically rewrites code without knowing the intention of the humans that produced that code.

[dmos62]

I've looked at black's style guide: https://black.readthedocs.io/en/latest/the_black_code_style/current_style.html and I really liked it. It should solve some pain points that I've been experiencing.

I'm antsy to try it out on our codebase to get a better picture of the effect. I don't feel like I have an hour to burn at the moment. If anyone would like to do it, go ahead!

[kgodey]

@dmos62 I'm very skeptical of black, see my previous comment. Please wait until I have time to work on this, hopefully in the next week or two. Once there's a concrete proposal for styling, it will be easier to discuss.

[dmos62]

@kgodey my idea is let's run black and look at the output. Then we have something to talk about.

I haven't looked at its style configuration, but it mentions that it purposefully doesn't offer a lot of options.

[kgodey]

@dmos62 We already did that early on in the project and then disabled it. I've also used it at previous workplaces.

[dmos62]

I'll include another link here to the code-style guidelines/tooling discussion thread, since we seem to be discussing this on two threads simultaneously: #1007.

[seancolsen]

Sorry, I wasn't aware of this ticket when I commented on #1007.

@kgodey said:

The issue with... auto-formatting is... that it automatically rewrites code without knowing the intention of the humans that produced that code.

I'm curious to hear more about your concern here. Can you provide an example of a case in which you find it helpful to communicate intent through code formatting? I also wonder whether Mathesar's python code might contain intent within formatting that fails to reach me because I typically do not attempt to infer intent from formatting while reading code. If we have conventions for communicating intent through formatting in our codebase, are those conventions documented anywhere?

[kgodey]

If we have conventions for communicating intent through formatting in our codebase, are those conventions documented anywhere?

@seancolsen We have no backend conventions documented anywhere. The action item for this issue is to first document backend style and formatting conventions, and then set up tooling to support those conventions.

I'm curious to hear more about your concern here.

I plan to create a draft of our backend style guide first before spending more time on this conversation; I think we'll have a more productive discussion when we actually have a concrete proposal to discuss. I'm reading through everyone's comments on this issue and on #1007 and I'll make sure to address them in the style guide. It might take me a couple of weeks because my current priority is getting the Views product work done.

[dmos62]

@kgodey you've mentioned disliking black for this. The rest of us aren't familiar with it though and it's such a low effort alternative that I think it's worth looking at (collectively). It would be nice to have its output around for comparison when discussing other options, for example.

[kgodey]

@dmos62 Turning on black is low effort but it has huge consequences across the codebase since it reformats everything and wipes out all existing formatting. Please have patience until the style guide is ready.

You are free to experiment with black or other tools locally if you want to see their output for comparison when discussing other options.

[dmos62]

Please have patience until the style guide is ready.

Didn't mean to hurry. I don't think this is a high priority.

[github-actions]

This issue has not been updated in 90 days and is being marked as stale.

[kgodey]

This is not a current priority, closing this.