Skip to content

[SPARK-29936][R] Fix SparkR lint errors and add lint-r GitHub Action #26564

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

Closed
wants to merge 7 commits into from
Closed

[SPARK-29936][R] Fix SparkR lint errors and add lint-r GitHub Action #26564

wants to merge 7 commits into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Nov 17, 2019

What changes were proposed in this pull request?

This PR fixes SparkR lint errors and adds lint-r GitHub Action to protect the branch.

Why are the changes needed?

It turns out that we currently don't run it. It's recovered yesterday. However, after that, our Jenkins linter jobs (master/branch-2.4) has been broken on lint-r tasks.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the GitHub Action on this PR in addition to Jenkins R and AppVeyor R.

@SparkQA

This comment has been minimized.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 18, 2019

Only one instance is left. It's a Window-specific function.

R/utils.R:6:5: warning: no visible global function definition for ‘shell’
    shell(scriptWithArgs, translate = TRUE, wait = wait, intern = wait)
    ^~~~~
lintr checks failed.

@SparkQA

This comment has been minimized.

R/pkg/.lintr Outdated
@@ -1,2 +1,2 @@
linters: with_defaults(line_length_linter(100), multiple_dots_linter = NULL, object_name_linter = NULL, camel_case_linter = NULL, open_curly_linter(allow_single_line = TRUE), closed_curly_linter(allow_single_line = TRUE))
linters: with_defaults(line_length_linter(100), multiple_dots_linter = NULL, object_name_linter = NULL, camel_case_linter = NULL, open_curly_linter(allow_single_line = TRUE), closed_curly_linter(allow_single_line = TRUE), object_usage_linter = NULL)
Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Nov 18, 2019

Choose a reason for hiding this comment

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

object_usage_linter has a limitation which cannot detect shell function used in Windows environment. Also, nolint doesn't work for this rule. This is a known limitation of lint-r itself.

@dongjoon-hyun
Copy link
Member Author

cc @srowen , @HyukjinKwon , @viirya , @felixcheung

@dongjoon-hyun
Copy link
Member Author

Also, cc @dbtsai

@SparkQA

This comment has been minimized.

@SparkQA
Copy link

SparkQA commented Nov 18, 2019

Test build #113973 has finished for PR 26564 at commit bfd29b9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-29936][R][TESTS] Add lint-r GitHub Action [SPARK-29936][R][TESTS] Fix SparkR lint errors and add lint-r GitHub Action Nov 18, 2019
@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 18, 2019

Now, this PR passes All tests (Jenkins/AppVeyor/GitHub Action including lint-r).

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-29936][R][TESTS] Fix SparkR lint errors and add lint-r GitHub Action [SPARK-29936][R] Fix SparkR lint errors and add lint-r GitHub Action Nov 18, 2019
@dongjoon-hyun
Copy link
Member Author

Could you review this PR, @HyukjinKwon ? Linter GitHub Action already passed.

@SparkQA
Copy link

SparkQA commented Nov 18, 2019

Test build #113980 has finished for PR 26564 at commit 22c1ba4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM if both tests pass

@dongjoon-hyun
Copy link
Member Author

Unfortunately, dev/lint-r.R triggers a full testing because it's categorized as root. Jenkins run will fail at midnight. It's a waste of time again. 😢

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 18, 2019

For the Jenkins, lint-r passed already on the on-going Jenkins run. For this PR, the rest of tests are irrelevant.

========================================================================
Running R style checks
========================================================================
Loading required package: methods

Attaching package: ���SparkR���

The following objects are masked from ���package:stats���:

    cov, filter, lag, na.omit, predict, sd, var, window

The following objects are masked from ���package:base���:

    as.data.frame, colnames, colnames<-, drop, intersect, rank, rbind,
    sample, subset, summary, transform, union


Attaching package: ���testthat���

The following objects are masked from ���package:SparkR���:

    describe, not

lintr checks passed.

@dongjoon-hyun
Copy link
Member Author

Thank you, @HyukjinKwon and @viirya ! This PR become much better than before.
Since lint-r passed on both Jenkins and GitHub Action, I'll merge this.
Merged to master.

@SparkQA
Copy link

SparkQA commented Nov 18, 2019

Test build #113987 has finished for PR 26564 at commit 7cb3be8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun dongjoon-hyun deleted the SPARK-29936 branch November 18, 2019 07:42
@felixcheung
Copy link
Member

felixcheung commented Nov 19, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants