Use black (or blue) for code style?

[whot]

I just spent way too long chasing down pylint and ruff issues (which were a bit hidden due to the magic env var) so now I'm wondering: would you be amenable to using black (or blue ¹)?

It's going to be one large commit to reformat everything but in the projects I've used it it's been a blessing for it to take care of almost all ruff/pylint cases - plus the consistent style (even if it's not always perfect)

In the CI it'd require one job to do black --check --diff . and fail if that fails. And black can quite easily be integrated into pre-commit as well for example.

Footnotes

1. haven't used blue myself but it's supposed to be more configurable than black. ↩

[martinpitt]

In principle I like the idea of black -- no arguing/worrying about formatting, etc. I'm even willing to make some concessions for that. I tried it maybe one or two years ago, and at least back then its priorities were too lopsided: it valued "short lines" as more important than anything else, which ruined readability and concise formatting. AFAIR back then one couldn't even adjust the line length. Although I agree that given its principles, the whole point is to not provide a lot of config options, but line length is something I have a stronger opinion about (what would life be without whim 😉 ).

E.g. this bit I don't like, as it makes the method declarations ragged, inconsistent, and harder to read, but that's in the class of "concession I'd grudgingly make":

 def load(mock, _parameters):
-    mock.AddMethods(MAIN_IFACE, [
-        ('GetActive', '', 'b', 'ret = self.is_active'),
-        ('GetActiveTime', '', 'u', 'ret = 1'),
-        ('SetActive', 'b', '', 'self.is_active = args[0]; self.EmitSignal('
-                               '"", "ActiveChanged", "b", [self.is_active])'),
-        ('Lock', '', '', 'time.sleep(1); self.SetActive(True)'),
-        ('ShowMessage', 'sss', '', ''),
-        ('SimulateUserActivity', '', '', ''),
-    ])
+    mock.AddMethods(
+        MAIN_IFACE,
+        [
+            ("GetActive", "", "b", "ret = self.is_active"),
+            ("GetActiveTime", "", "u", "ret = 1"),
+            (
+                "SetActive",
+                "b",
+                "",
+                "self.is_active = args[0]; self.EmitSignal(" '"", "ActiveChanged", "b", [self.is_active])',
+            ),
+            ("Lock", "", "", "time.sleep(1); self.SetActive(True)"),
+            ("ShowMessage", "sss", "", ""),
+            ("SimulateUserActivity", "", "", ""),
+        ],
+    )

similar to this:

-    mock.AddMethods(MAIN_IFACE, [
-        ('GetCapabilities', '', 'as', f'ret = {caps!r}'),
-        ('CloseNotification', 'i', '', 'if args[0] < self.next_id: self.EmitSignal('
-                                       '"", "NotificationClosed", "uu", [args[0], 1])'),
-        ('GetServerInformation', '', 'ssss', 'ret = ("mock-notify", "test vendor", "1.0", "1.1")'),
-        ('Notify', 'susssasa{sv}i', 'u', '''if args[1]:
+    mock.AddMethods(
+        MAIN_IFACE,
+        [
+            ("GetCapabilities", "", "as", f"ret = {caps!r}"),
+            (
+                "CloseNotification",
+                "i",
+                "",
+                "if args[0] < self.next_id: self.EmitSignal(" '"", "NotificationClosed", "uu", [args[0], 1])',
+            ),
+            ("GetServerInformation", "", "ssss", 'ret = ("mock-notify", "test vendor", "1.0", "1.1")'),
+            (
+                "Notify",
+                "susssasa{sv}i",
+                "u",
+                """if args[1]:

but that's the bit which I consider overzealous vandalism:

-    def Set(self, interface_name: str, property_name: str, value: Any, *_, **__) -> None:
+    def Set(
+        self, interface_name: str, property_name: str, value: Any, *_, **__
+    ) -> None:

Fortunately by now, black has a line length option, and with -l 118 (which is my goal -- 120 terminal columns with two columns reserved for ALE markers in vim) that particular instance goes away.

I also don't like how it insists on using " everywhere. ' is easier to type, and in other projects I use the two to differentiate human-visible strings (") from identifiers. However, there are hardly any human-visible strings in dbusmock, so I can live with this.

So TL/DR: I'm not a huge fan admittedly -- ruff and flake8 seem quite alright. But if you feel like you want to do this, please go ahead. You are the most active contributor and have earned the rights to have some whims on your own 😀

I don't have any experience with blue. My completely uninformed gut feeling is that I'd rather stay with black as long as we can both live with its output -- but no strong opinion at all.

[whot]

The " vs ' annoyed me initially as well but at this point I'm so used to the double-quotes that I get confused in python code not using that :) The main selling point of black only kicks in once you have most python repos that you interact with using it, then the code all looks the same without per-project quirks.

If the quotes are one large concern I think we could use blue with single-quotes enabled but I don't think blue has the integrations everywhere.

I'll think about this again after #184 is merged, I really don't want to mess things up before that :)

[martinpitt]

FTR, the quotes aren't a concern for me in dbusmock in particular -- there's no consistency about them right now. They would be a blocker in cockpit, where we paid much more attention to them.