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

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

Merged
merged 2 commits into from Nov 25, 2018

Conversation

@jameslamb
Copy link
Contributor

@jameslamb jameslamb commented Nov 23, 2018

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.

Copy link
Member

@StrikerRUS StrikerRUS left a comment

One minor note from my side.
I suppose that NEWS should be updated. Right, @mlampros ?

@jameslamb
Copy link
Contributor Author

@jameslamb jameslamb commented Nov 23, 2018

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

@StrikerRUS StrikerRUS commented Nov 24, 2018

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

@mlampros
Copy link
Collaborator

@mlampros mlampros commented Nov 24, 2018

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

@jameslamb jameslamb commented Nov 24, 2018

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

@mlampros mlampros commented Nov 25, 2018

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

@jameslamb jameslamb commented Nov 25, 2018

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

Copy link
Collaborator

@mlampros mlampros left a comment

thanks

Copy link
Member

@StrikerRUS StrikerRUS left a comment

Thanks a lot for your contribution!

@StrikerRUS StrikerRUS merged commit 026c670 into RGF-team:master Nov 25, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.