JuliaML / LIBSVM.jl

LIBSVM bindings for Julia
Other
88 stars 35 forks source link

GH Action: install Julia formatter action #81

Open iblislin opened 3 years ago

codecov-commenter commented 3 years ago

Codecov Report

Merging #81 (39ecd19) into master (56e9a2e) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #81   +/-   ##
=======================================
  Coverage   84.75%   84.75%           
=======================================
  Files           5        5           
  Lines         223      223           
=======================================
  Hits          189      189           
  Misses         34       34           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 56e9a2e...39ecd19. Read the comment docs.

iblislin commented 3 years ago

@barucden could you try out the style configure?

barucden commented 3 years ago

@barucden could you try out the style configure?

Sure. I tried running the commands locally, and one thing "bothers" me. It seems that the formater always splits the arguments of an invoked function into multiple lines. For example

-    return ccall((:svm_train, libsvm), Ptr{SVMModel},
-                 (Ref{SVMProblem}, Ref{SVMParameter}),
-                 problem, param)
+    return ccall(
+        (:svm_train, libsvm),
+        Ptr{SVMModel},
+        (Ref{SVMProblem}, Ref{SVMParameter}),
+        problem,
+        param,
+    )

Perhaps it is not always necessary? Also notice the comma after the last argument.

Is it possible to allow only selected format adjustments? E.g., to adjust only spaces around = and indentation, and leave the rest (such as splitting function arguments into multiple lines) to the contributors.

iblislin commented 3 years ago

Is it possible to allow only selected format adjustments? E.g., to adjust only spaces around = and indentation, and leave the rest (such as splitting function arguments into multiple lines) to the contributors.

ATM, seems there isn't a config of enabling single rule only. The full list of configs is here: https://domluna.github.io/JuliaFormatter.jl/dev/api/#JuliaFormatter.format_text-Tuple{AbstractString}

I'm going the try out CustomStyle (https://domluna.github.io/JuliaFormatter.jl/dev/custom_styles/), and check if we can ignore ccall only or not.

barucden commented 3 years ago

I tried the YAS style. It does not split the argument list into multiple lines, which is nice. However, it is still difficult to set the appropriate row width. I personally try to keep each row 80 letters wide at most, except for the cases when I don't :) Sometimes I break this rule because it is just easier to read. I might be too conservative, but enforcing strictly defined formatting rules is perhaps unnecessarily inflexible.

How about just publishing a "style guide"? The YAS style, for instance, looks quite alright to me. The development here is not that wild after all :)

iblislin commented 3 years ago

How about just publishing a "style guide"? The YAS style, for instance, looks quite alright to me. The development here is not that wild after all :)

hmm, seems it's the more suitable way at this moment. Let's pick YAS as the guideline for this repo. :+1: And yes, it's just a guideline, the rules can be broke if more readable.

Actually, the goal of formatter is to reduce the time spent on maintaining styles. I want a formatter that can remove whitespace automatically, so I can just accept PRs than let the bot do the rest of works. (I'm tired of sending some removing-space PR. :p).

I like your idea about enabling the only selected syntax transformation. I will try to work on that later if I have enough time bandwidth.

barucden commented 3 years ago

Yeah, if it could be configured to just fix white-spaces, that would be great!