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.

Sign up for GitHub

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

Jump to bottom

[R] change internal calls to use keyword args #267

Merged
merged 4 commits into from
Nov 26, 2018
Merged

[R] change internal calls to use keyword args #267

merged 4 commits into from
Nov 26, 2018

Conversation

jameslamb
Copy link
Contributor

This PR addresses #244

Please do not review until we've merged #266 . I branched off of that PR to avoid dealing with merge conflicts. If/when we merge that, I'll rebase this PR and the diff will make more sense.

@StrikerRUS
Copy link
Member

@jameslamb Thank you very much!

Unfortunately, Travis is unhappy. Can you please take a look?

@jameslamb
Copy link
Contributor Author

yep! The fact that it passed on appveyor tells me it's probably just one of those transient travis things. I'll check.

@jameslamb
Copy link
Contributor Author

Ha I was too quick to blame Travis. Totally my fault! The package uses occurences (one r) but I accidentally used occurrences (2 r's) in one place. Should be fixed as of my most recent commit

@StrikerRUS
Copy link
Member

We haven't configured Appveyor for checking R-package yet :-(

Seems to be another one typo:
'tree_grain_ratio' -> 'tree_gain_ratio' (grain -> gain)

@jameslamb jameslamb mentioned this pull request Nov 24, 2018
Closed
@jameslamb
Copy link
Contributor Author

jameslamb commented Nov 24, 2018 •
edited

whoops! Fixed. I also created #268 so anyone coming to the repo knows appveyor setup is a contribution that would be welcomed.

@jameslamb
Copy link
Contributor Author

Rebasing right now, now that we've merged #266

@jameslamb
Copy link
Contributor Author

ok @StrikerRUS @mlampros this is ready for review!

StrikerRUS
StrikerRUS approved these changes Nov 25, 2018
View reviewed changes
Copy link
Member

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM!
Please document this in NEWS file.

@jameslamb
Copy link
Contributor Author

Oh sure, no prob. Just updated the NEWS.md

mlampros
mlampros approved these changes Nov 25, 2018
View reviewed changes
Copy link
Collaborator

@mlampros mlampros left a comment

Choose a reason for hiding this comment

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

thanks

@StrikerRUS StrikerRUS mentioned this pull request Nov 26, 2018
Closed
@StrikerRUS StrikerRUS merged commit 43aae31 into RGF-team:master Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlampros mlampros mlampros approved these changes

@StrikerRUS StrikerRUS StrikerRUS approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jameslamb @StrikerRUS @mlampros