search

JuliaML / LIBSVM.jl

LIBSVM bindings for Julia
Other
88 stars 35 forks source link

rename functions involving side effect #80

Closed iblislin closed 3 years ago

iblislin commented 3 years ago 
libsvm_set_verbose     -> libsvm_set_verbose!
libsvm_set_num_threads -> libsvm_set_num_threads!
set_num_threads        -> set_num_threads!
codecov-commenter commented 3 years ago

Codecov Report

Merging #80 (11508e6) into master (56e9a2e) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #80   +/-   ##
=======================================
  Coverage   84.75%   84.75%           
=======================================
  Files           5        5           
  Lines         223      223           
=======================================
  Hits          189      189           
  Misses         34       34
Impacted Files Coverage Δ
src/libcalls.jl 85.71% <ø> (ø)
src/LIBSVM.jl 93.93% <100.00%> (ø)

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...11508e6. Read the comment docs.

barucden commented 3 years ago

I am not sure about the naming. Sure, the methods do have side effects, but they don't modify their arguments. I.e. I would expect that function libsvm_set_verbose!(v) would modify v. Of course, that might be just me...

iblislin commented 3 years ago

Sure, the methods do have side effects, but they don't modify their arguments.

Yeah, the c function global states is a tricky one. I'm fine with original naming. I just re-read this: https://docs.julialang.org/en/v1/manual/style-guide/#bang-convention