dfmt icon indicating copy to clipboard operation
dfmt copied to clipboard

dfmt v0.10.0-beta.1 array formatting

Open LaurentTreguier opened this issue 5 years ago • 2 comments

I ran dfmt v0.10.0-beta.1 on dls' code base to see how it changes. I always use dfmt to format it, so it's currently formatted with dfmt v0.9.0, and any change when formatting with a newer version should correspond to a change since v0.9.0. Some observations:

  • Ternary operators are better formatted (at least in the cases I've seen, it looks slightly better) This is caused by 6e4136a353f737ead16bd74636efd6a8c0d63898; since it's in the middle of commits about arrays and AA's it looks like it's a side effect (correct me if that's wrong). Example:
         override void visit(const UnionDeclaration dec)
         {
    -        visitSymbol(dec, SymbolKind.interface_, true, dec.structBody is null ? 0
    -                : dec.structBody.endLocation);
    +        visitSymbol(dec, SymbolKind.interface_, true, dec.structBody is null
    +                ? 0 : dec.structBody.endLocation);
         }
    
  • When a line is long and contains array/AA literals, they are formatted on multiple lines somewhat unnecessarily. Examples:
     unittest()
     {
         // [...]
    -    assert(convertFromJSON!(int[string])(JSONValue([16, 42])) == ["0" : 16, "1" : 42]);
    +    assert(convertFromJSON!(int[string])(JSONValue([16, 42])) == [
    +            "0": 16,
    +            "1": 42
    +            ]);
     }
    
         if (!SymbolTool.instance.workspacesFilesUris.canFind!sameFile(uri))
         {
    -        send(TextDocument.publishDiagnostics, new PublishDiagnosticsParams(uri, []));
    +        send(TextDocument.publishDiagnostics, new PublishDiagnosticsParams(uri, [
    +                ]));
         }
    
  • Large array and AA declarations are perfectly formatted however, as intended, and don't need //dfmt off and //dfmt on anymore

Full diff with the changes that I've found: dfmt.diff.txt

LaurentTreguier avatar Feb 17 '19 11:02 LaurentTreguier

maybe some additional heuristics should be added (like for example if the array fits in one single line after wrapping)

I think in general more cases look better now than before, but these minor bugs are slightly buggin me.

diff --git a/source/dls/protocol/jsonrpc.d b/source/dls/protocol/jsonrpc.d
index d0a7a44..ac22838 100644
--- a/source/dls/protocol/jsonrpc.d
+++ b/source/dls/protocol/jsonrpc.d
@@ -176,7 +176,10 @@ private void send(T : Message)(T m)
 
     synchronized
     {
-        foreach (chunk; ["Content-Length: ", text(messageString.length), eol, eol, messageString])
+        foreach (chunk; [
+                "Content-Length: ", text(messageString.length), eol, eol,
+                messageString
+            ])
         {
             communicator.write(chunk.toUTF8());
         }

I think this is OK but it could have been formatted smaller

WebFreak001 avatar Feb 17 '19 11:02 WebFreak001

(sorry last comment sent early)

@@ -141,8 +139,8 @@ InitializeResult initialize(InitializeParams params)
                     && textDocCaps.rename.prepareSupport.get();
 
                 renameProvider = initOptions.capabilities.rename ? prepareSupport
-                    ? convertToJSON(new RenameOptions(true.nullable))
-                    : JSONValue(true) : JSONValue(false);
+                    ? convertToJSON(new RenameOptions(true.nullable)) : JSONValue(
+                            true) : JSONValue(false);
             }
         }

yes the ternary operator was a side effect, but it's hard to make it work with both at once.

@@ -217,7 +214,9 @@ void initialized(JSONValue nothing)
             auto registration = new Registration!DidChangeWatchedFilesRegistrationOptions("dls-file-watchers",
                     "workspace/didChangeWatchedFiles", registrationOptions.nullable);
             send(Client.registerCapability,
-                    new RegistrationParams!DidChangeWatchedFilesRegistrationOptions([registration]));
+                    new RegistrationParams!DidChangeWatchedFilesRegistrationOptions([
+                            registration
+                        ]));
         }

same as your example with the [] now wrapping, I think there needs to be some more logic if something will actually wrap

--- a/util/source/dls/util/json.d
+++ b/util/source/dls/util/json.d
@@ -119,7 +119,9 @@ unittest
     assert(testClass.floating == 3.0);
     assert(testClass.text == "Hello world");
     assert(testClass.array == [0, 1, 2]);
-    immutable dictionary = ["key1" : "value1", "key2" : "value2", "key3" : "value3"];
+    immutable dictionary = [
+        "key1" : "value1", "key2" : "value2", "key3" : "value3"
+    ];
     assert(testClass.dictionary == dictionary);
     assert(testClass.testStruct.uinteger == 16);
     assert(testClass.testStruct.json["key"].str == "value");

this shouldn't be like this. It seems the AST information used for helping finding AAs didn't trigger properly here. (there should be no spaces before the :, so it doesn't know we are in an AA and thinks it's a a normal array)

WebFreak001 avatar Feb 17 '19 11:02 WebFreak001