pretty code forever in 2023?

[bug-or-feature]

Happy new year all. I can't remember who it was last year whose Santa wish was for the code to be Blacked. But when their wish was granted I remember feeling like I'd received a nice late Christmas present too.

One year on, and the code needs another clean; currently around 200 files would be changed. But maybe this time we can make it stay pretty forever? I've been looking into potential solutions.

Ad hoc

We could just do an occasional Black run over the codebase. Every week? Or month? It's not ideal though; automated would be better

GitHub action

We could setup a GitHub action that runs on every push or pull request. This would fail the build if the submitted code was not already Blacked. So, a bit like if someone submits a code change that causes a unit test to fail. This would be good for PRs -@robcarver17 could reject the PR until fixed. But, not so good for your own changes Rob, at least not when you commit direct onto the master branch. The dirty code would still be in the repo, and it would take another commit to make it clean again. Annoying

What we really wanto is a way to make sure no dirty code ever gets into the repo. Some ideas:

Black as a server

Black can be installed locally as a mini HTTP server. You can then configure
PyCharm (and other IDEs and editors) to automatically run Black over your code
as you save it. Steps here.To use Black like this would require another project config file at the top level, pyproject.toml, to configure files to be ignored etc. I don't think it can be done in setup.py

Git hook

On commit (to your local repo), git can be configured to run black over the changed code. Requires an extra dependency on pre-commit, but for your local environment only, not the project itself. Steps here

@robcarver17 what do you think? I'm happy to drive this if you approve. Maybe read this for a potential argument against

[bug-or-feature]

There's another tool worth a mention:

https://deepsource.io/

It's a service you can hook up to your Github repo, and it helps identify potential issues. It can fix a bunch of code quality issues, automatically creating PRs. One of the many things it can do is to automatically Black any incoming submissions.

I've been playing around with it in another repo, its very comprehensive. And free for open source projects. I suspect it would need time and effort commitment from the repo Admin, so may not be good for this project right now

[robcarver17]

I support this idea; it saves me having to periodically Black everything (it's not really practical for someone else to do it, since it produces an enormous diff list and I can't really properly check it - although I trust you guys, there is always a danger someone could steal your credentials and drop a dodgy change into a massive Black refactoring).

[bug-or-feature]

Great. I'd propose the following steps:

1. I'll create an initial PR. Don't merge yet. It will include:
   • config file for the GitHub action
   • pre-commit config
   • pyproject.toml file
   • a new top level file 'CONTRIBUTING.md' that documents stuff new developers need to know
2. You run Black on the whole code base, commit
3. Note the commit identifier and write to a new file .git-blame-ignore-revs as described here. Commit that
4. Merge my PR
5. We all enjoy pretty code

Do you use any custom Black options, eg line length?

[robcarver17]

I'd personally like a 90 char line length, since I usually compose on my 14" laptop with massive fonts to compensate for my poor eyesight, with the pycharm project window.that takes out about 20 characters worth of width.

EDIT: I think the default is actually 88, so that's fine.

[emretezel]

Should we just ask ChatGPT to re-write the whole thing :)

[ehiggs]

We could just do an occasional Black run over the codebase. Every week? Or month? It's not ideal though; automated would be better

There is already a pre-commit-config.yml with a black hook. This means when someone makes a commit, black should be run (if they ran pre-commit install when making their venv. If contributors are forgetting to do this, then it would be useful to have a setup_venv.sh or create_venv.sh script that can install pre-commit and the other tools used in pre-commit (flake8, autoflake, isort, pyupgrade, mypy, ..., fill your boots)

Then, if a file is not blacked when it is submitted as a PR, a Github action should fail the PR until it's fixed. Is it not working?

FWIW there is also darker which applies Black to only the parts that have changed. This makes the diffs in PRs smaller since whole files being formatted are limited to only the parts being changed.

[robcarver17]

There does seem to be an issue getting my local black, and the github black to agree.

If this can't be fixed: Now I'm running black on everything locally, as every other commit has to be a PR, if @ehiggs is correct and we can force commits to be blacked, then wouldn't that be okay? And we turn off the constant lint errors that will appear every time I commit?

[robcarver17]

rob@RobLaptop:~$` black --version
black, 21.12b0 (compiled: no)

[robcarver17]

I've just reverted to '22.12.0' precisely to see if that helps.

[robcarver17]

OK that seems to have done the trick.

[bug-or-feature]

awesome

[bug-or-feature]

Black claim their style is stable. The current config in GitHub will automatically pick up new releases. So developers would need to keep local versions in line. We may need to tweak the GitHub config to a fixed version if it proves a problem

[tgibson11]

Below is a copy my comment from PR 927.

For context, I said I had mixed feeling about black. @bug-or-feature asked about my concerns. The following was my response.

==========

Well, to put it briefly, I think it's going to annoy me. It increases friction for pull requests, so I'll be marginally less likely to do them.

I've been writing software for a long time, and I've always placed a high value on code that is clean, easy to understand, and well-formatted.

Suggestions are great. I find the PEP 8 style warnings in PyCharm very useful. But it feels slightly insulting to have software critiquing my code style to the extent that commits or PRs will be rejected because of a trivial disagreement about style.

I will admit that my opinion is more or less "rules for thee, but not for me", so take it with a grain of salt.

[robcarver17]

The rules make sense for me, since although I have been writing software for a long time, I'm not always consistent on the formatting side of things, and I'm not that bothered about exactly what the code looks like; having an enforced external standard makes sense to me. But I can definitely see your point, and I don't want to turn off potential contributors.

@tgibson11 What was the workflow like with the new lint check, did you have to make a lot of changes before it worked?

@bug-or-feature is there a way to make this easier, eg get the PR to be formatted 'on the way in' without any work by the committer?

I guess my thoughts are that since I do most of the commits, and I'm now autoblacking everything I edit, everything will end up being blacked eventually even if it isn't 100% black when committed. And most of the other people committing are going to commit nice code, even if it doesn't conform to the hardcore black standard on the initial PR. If someone has a PR that is truely ugly, I'd reject it.

[tgibson11]

My workflow so far has been:

0. Read black style guide
1. Locally, same as before (no black), just pay extra attention to formatting
2. Create PR
3. See what happens
4. If black isn't happy, see what it didn't like
5. Do a quick commit to fix it

My PRs are usually small, so this seems manageable, just a bit of extra work. My first one went smoothly.

My last one (927) had one issue where black wanted a string formatting operation on a separate line from the function call for which it was an argument. Overall, it probably took less than a minute to identify and correct the issue.

[bug-or-feature]

@tgibson11 would definitely not want to discourage anyone from contributing. Was it not an option for you to use the local git hook, or the IDE auto save thing?

@robcarver17 there are git server side hooks, which can be configured to do the work 'on the way in'. But unfortunately they're not supported on GitHub.

[tgibson11]

I briefly looked at those options, but they both seemed like more work to setup (and probably maintain going forward) than they would be worth to me.

Like I said, I like to write nicely formatted code in the first place. Honestly, I probably prefer to internalize the black rules and apply them myself.

BTW, my original comment wasn't really intended to spark a big discussion. This is Rob's repo after all, and I'm happy to follow whatever rules he wants to make. I'll get used to it.

[robcarver17]

FWIW it only took me a couple of minutes to setup BlackConnect to blackd on pycharm, but appreciate that mileage may vary on other IDE.

[tgibson11]

If PyCharm or git came pre-packaged with black, where I could just flip a switch to enable it, I would use that.

Installing new software and plugins is a step further than I'm prepared to go for now.

[cholonam]

FWIW I use VSCode for development and it is very easy to set it up to run black every time you save a file (I also usually have several linters configured, flake8, mypy...). I have never used PyCharm, but can't understand why is it so difficult to setup something as trivial as a code formatter. Maybe you want to give VSCode a try..

[tgibson11]

I'm not saying it is difficult, only that I don't want to do it. Just roll your eyes and consider me a stubborn old man.

[robcarver17]

294 files would be reformatted, 242 files would be left unchanged.

And yet:

rob@RobLaptop:~$ black pysystemtrade/
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install black[jupyter]``
All done! ✨ 🍰 ✨
536 files left unchanged.

[bug-or-feature]

Black has a new version for 2023 that changes their stable idea of what pretty should look like, and our CI build pulls the latest. Not so stable if you start using it in December... we were a bit unlucky with timing I guess. I'll create a PR to pin the versions so we don't get this again