JuliaAI / DecisionTree.jl

Julia implementation of Decision Tree (CART) and Random Forest algorithms
Other
342 stars 101 forks source link

Apply JuliaFormatter #219

Closed rikhuijzer closed 1 year ago

rikhuijzer commented 1 year ago

The formatting of DecisionTree.jl is sometimes inconsistent and using styles which are generally advised against such as aligning items vertically. This PR applies JuliaFormatter to the repository to get a coherent style. I've also tried to replace calls such as f(; x = x) by f(; x). This is possible since Julia 1.5 (NEWS.md) and this repository has a lower bound on Julia 1.6.

The command that I used to format the repository is:

julia> using JuliaFormatter

julia> format(".", BlueStyle(); always_use_return=false, import_to_using=false)

I've chosen the blue style because it's a pretty well-known and well thought out style, see the BlueStyle repo for details.

We could also add a .JuliaFormatter.toml file to the root of the repository containing:

style = "blue"
always_use_return = false
import_to_using = false

To use the newer keyword arguments syntax, that is replace f(; x = x) by f(; x), I've used comby:

$ comby -matcher .jl '(:[a]; :[b], :[c]=:[c])' '(:[a]; :[b], :[c])'

and some variations of this command.

I've also considered adding a CI workflow to check whether PRs satisfy the code style. However, this would be an extra burden to contributors so I think that an occasional run of the JuliaFormatter should be enough.

codecov-commenter commented 1 year ago

Codecov Report

Merging #219 (a49141b) into dev (7591df6) will decrease coverage by 0.66%. The diff coverage is 89.84%.

:exclamation: Current head a49141b differs from pull request most recent head 413a2d9. Consider uploading reports for the commit 413a2d9 to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev     #219      +/-   ##
==========================================
- Coverage   88.15%   87.50%   -0.66%     
==========================================
  Files          10       10              
  Lines        1266     1328      +62     
==========================================
+ Hits         1116     1162      +46     
- Misses        150      166      +16     
Impacted Files Coverage Δ
src/scikitlearnAPI.jl 51.33% <53.12%> (-0.34%) :arrow_down:
src/DecisionTree.jl 61.44% <55.55%> (+1.44%) :arrow_up:
src/regression/main.jl 88.88% <84.61%> (+0.42%) :arrow_up:
src/util.jl 90.78% <92.57%> (+0.44%) :arrow_up:
src/regression/tree.jl 95.03% <96.89%> (+0.07%) :arrow_up:
src/classification/tree.jl 94.89% <96.96%> (+0.07%) :arrow_up:
src/measures.jl 97.39% <97.01%> (+0.04%) :arrow_up:
src/classification/main.jl 95.91% <97.36%> (-0.26%) :arrow_down:
src/abstract_trees.jl 91.66% <100.00%> (+0.36%) :arrow_up:
src/load_data.jl 91.30% <100.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ablaom commented 1 year ago

We could also add a .JuliaFormatter.toml file to the root of the repository containing:

style = "blue" always_use_return = false import_to_using = false

No objection to adding this.

I tend to agree adding to CI is unnecessary but don't have a strong opinion about it. Someone who does is welcome to open a new PR.

ablaom commented 1 year ago

@rikhuijzer Thanks for this contribution.