Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH Action: install Julia formatter action #81

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

iblislin
Copy link
Member

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2021

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
Copy link
Member Author

@barucden could you try out the style configure?

@barucden
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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. 👍 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
Copy link
Member

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

Copy link
Contributor

@ablaom ablaom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea to me.

@ablaom ablaom self-requested a review April 18, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants