Re: [BuildStream] [Development topic] Use black to format code

[Chandan Singh <chandan chandansingh net>]

Hi Darius, Tom,

Thanks for taking the time to review the proposal. Some comments below.

> I for one am definitely in favour of introducing Black to the
> Buildstream source, and can live with an initial 'cleanup' commit.
> However, how do you propose the use of Black going forward? Do you
> expect it to become a manual part of the contributors workflow, or part
> of some pre-commit/merge automation? I've also seen projects using Black
> for scheduled blanket format commits, I can't say I'm too keen on that
> though.

These are fair questions. I didn't talk about the tooling much in the initial
message as I was trying to gauge interest in the idea first. Here are my
thoughts on how this would work:

  * We provide 2 tox environments - format and format-check. The `format`
    environment will format the code using `black`. In future, if we add any
    other formatters (like isort), they could presumably be run in the same
    environment. The `format-check` environment should do similar things as the
    `format` environment, but instead of rewriting the files, it would just
    output what would have been changed.

  * We expect developers to submit already formatted code along with their
    patches. This could be done easily with something like `tox -e format`.

  * We add a new CI job (or modify an existing one) to run the `format-check`
    environment as a check, similar to how we currently run the linters.

If we follow something like above, we shouldn't need blanket reformatting
commits from an automation. Rather, our codebase would be formatted correctly
at all points in history.

There's one exception to the above rule - when Black decides to change some
formatting style. This doesn't happen very often but can happen as the
formatter isn't considered stable just yet. To mitigate this, we can pin the
Black version number to ensure that we keep getting stable output. And, we can
bump it consciously on a periodic basis. Again, this would behave similar to how
we pin versions of our other dependencies.

Cheers!
Chandan

On Tue, Nov 5, 2019 at 2:19 PM Darius Makovsky via buildstream-list
<buildstream-list gnome org> wrote:
> On 05/11/2019 14:07, Darius Makovsky via buildstream-list wrote:
>
> > I'm opposed to blanket format commits: I don't know anyone who reads
> > them and we're entirely relying on a tool for that. I suggest if it's
> > going to replace pycodestyle then it should be part of the pipeline
> > for MRs and it could also be a pre-commit hook.
>
> Sorry, to clarify, I'm suggesting if an MR contains source which would
> be changed by running black then the check should fail. This would
> assert that formatting is being performed on the code in the MR.
>
> --
> Best Regards,
> Darius
>
>
> For Codethink's privacy-policy please see
> https://www.codethink.co.uk/privacy.html
> _______________________________________________
> buildstream-list mailing list
> buildstream-list gnome org
> https://mail.gnome.org/mailman/listinfo/buildstream-list