SethBling / cbscript

CBScript for Minecraft
1.13k stars 28 forks source link

Black code formatting #33

Open CoolCat467 opened 2 months ago

CoolCat467 commented 2 months ago

This pull request formats all of the code using the Black code formatter, as well as automatically removing unused imports. The one functional change I made manually was that in unittest/unit_test.py, in test_selector_expr, there was an invalid lambda function that was stopping automatic formatting from working. Other than that, all changes in this pull request are automatic or adding configuration files that drive automatic changes.

Main details about Black from its README:

Black is the uncompromising Python code formatter. By using it, you agree to cede control over minutiae of hand-formatting. In return, Black gives you speed, determinism, and freedom from pycodestyle nagging about formatting. You will save time and mental energy for more important matters.

Blackened code looks the same regardless of the project you're reading. Formatting becomes transparent after a while and you can focus on the content instead.

Black makes code review faster by producing the smallest diffs possible.

The main reason to use it is that it makes everything formatted in a clean way, makes diffs smaller and more readable, and eliminates arguments about code styling.

I've set automatic formatting to work via git pre-commit hooks by using the pre-commit project, which makes using them trivial via .pre-commit-config.yaml, which even has free continuous integration support via pre-commit.ci which can do things like automatically formatting new pull requests.

I am planning on making more pull requests based off of the changes in this one, but I think consistent formatting is a must for any project in my opinion.

CoolCat467 commented 2 months ago

I know that reviewing 259 files of changes to nearly the entire file is an incredible thing to ask, so if you need me to separate out changes I manually made vs automatic changes please don't hesitate to tell me.

ckohen commented 2 months ago

The switch from tabs to spaces is probably a large majority of the changes, and is something I will strongly push back against. Tabs are far superior than spaces for indentation, and it's basically the entire reason they exist in the first place. Not to mention the accessibility benefits it has. You (or black) seems to have chosen that 1 tab = 4 spaces, and that's not how I like to read my code. Why shouldn't we be able to make that decision ourselves?

CoolCat467 commented 2 months ago

This is something Black inherits from following PEP8, which is the gold standard of sorts for Python code. Please give it a read, it's interesting to see their reasoning behind their choices.

The fight of Tabs vs. Spaces is a very hot debate for some people, and I would rather avoid it. A lot of the entire purpose of black is to normalize formatting by everyone to be similar so we don't have fights like this. The goal of contributing to open source software it to make things work better. Black is trying to make this easier by eliminating fights over stylistic choices.

When I first started using black, I really didn't like the fact it makes string literals use single quotes, and how it changes everything to use double quotes. After a while of working with it though, you get used to it because even if you type your code using single quotes it will automatically fix it for you. I would imagine something similar might happen with tab based code.

ckohen commented 2 months ago

Unfortunately you've brought it up by making this PR. PEP8 makes literally no argument for it. I'd love to see one, but there is nothing in that document that makes arguments, just statements.

The string arguments I totally get, I write primarily typescript, and there's plenty of things that go both ways. The tabs vs. spaces is totally different though. It will almost certainly change tabs to spaces when it fixes it, or more likely your editor will insert spaces in the first place (especially since you can't mix them anymore).

Tab represents 1 indent, unless we agree upon the number of spaces that an indent is (which I don't agree with 4) then this makes DX way worse for anyone that doesn't agree. As far as I can tell, the reasoning behind spaces in PEP8 is "because everyone already did it that way". Should we keep doing something in an objectively worse way because everyone else is (insert classic bridge jump argument)?