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-package] fix partial matching of keyword arguments in lgb.cv() (fixes #3629) #3630

Merged
merged 5 commits into from
Dec 7, 2020

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Dec 5, 2020

This PR fixes #3629 , and fixes the R package CI to be sure that similar issues don't arise in the future.

Also fixes #3616

For CI, I'm proposing that instead of having a limit on the NUMBER of R CMD CHECK NOTES, we just ignore very specific NOTEs that we know are not problematic for CRAN. I think that will be less susceptible to issues like #3629 and give us more confidence that CI is catching problems that would be caught by CRAN.

This is blocking release 3.1.1 (#3611)

@@ -186,7 +195,7 @@ if grep -q -R "WARNING" "$LOG_FILE_NAME"; then
exit -1
fi

ALLOWED_CHECK_NOTES=2
ALLOWED_CHECK_NOTES=0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great to see that we can simplify NOTEs checking by just some exports!

I think we can now change this to simple if grep -q -R "NOTE ...", can't we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh sure, I can do that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

while we're thinking about it, I think we should add a fix for #3616 in this PR too:

if grep -q -R "ERROR" "$LOG_FILE_NAME"; then
    echo "ERRORs have been found by R CMD check!"
    exit -1
fi

Copy link
Collaborator

@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.

Thanks!

Just one minor suggestion to simplify and speedup the code for your consideration: use one grep -E "NOTE|WARNING|ERROR" ... instead of three separated ifs (and similar for PowerShell).

@jameslamb
Copy link
Collaborator Author

Thanks!

Just one minor suggestion to simplify and speedup the code for your consideration: use one grep -E "NOTE|WARNING|ERROR" ... instead of three separated ifs (and similar for PowerShell).

I like it! Added in d9ca1c4. I tested on may Mac with bash and pwsh.

@jameslamb jameslamb merged commit f38f118 into microsoft:master Dec 7, 2020
@jameslamb jameslamb deleted the fix/partial-match branch December 7, 2020 06:28
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants