search

RGF-team / rgf

Home repository for the Regularized Greedy Forest (RGF) library. It includes original implementation from the paper and multithreaded one written in C++, along with various language-specific wrappers.
378 stars 58 forks source link

[R] broke code out into individual files (fixes #243) #266

Closed jameslamb closed 6 years ago

jameslamb commented 6 years ago

Per #243, in this PR I propose breaking out classes and functions into individual script files. This can make the code a little easier to navigate and diffs in PRs easier to read.

jameslamb commented 6 years ago

it depends on the purpose of NEWS.md. This is not a user-facing change (i.e. no one who installs that package has to care about it).

StrikerRUS commented 6 years ago

@jameslamb That's true. Leaving the decision for @mlampros, since he is the maintainer of R-package.

mlampros commented 6 years ago

From the perspective of a user the only case that I can think, is when someone intends to search for a code snippet in the files. Previously, for instance I had to search in 3 files whereas now in multiple. On the other hand if someone intends to use the functions of the package (after installation) then he/she will not see any difference at all. I mostly write such changes in the NEWS.md file, but I guess, now I'm not the only contributor for the R package in the RGF-team repository.

jameslamb commented 6 years ago

IMHO a non-developer "user" doesn't need to ever look at scripts or search through them, since you can always run a function without parentheses in R and see the source code printed in the console.

Either way, if you tell me to update NEWS I'll update it!

mlampros commented 6 years ago

It depends. When I started learning R I didn't know that I was able to run a function without parentheses, as you said, so that I can have a look to the code. Moreover, I think it's also a matter of preference. Some people prefer to view the code in the files directly (like myself) and others don't. I would suggest that we add this change in NEWS.md file, if you don't have anything against.

jameslamb commented 6 years ago

Makes sense to me! I just added the update to NEWS.md