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

Resolve error with purrr::keep in lintr #2887

Closed
wants to merge 3 commits into from

Conversation

nwiltsie
Copy link
Contributor

Fixes #2886.

Proposed Changes

  1. Pass an anonymous function to purrr::keep rather than a formula as recommended by the docs

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@nvuillam
Copy link
Member

nvuillam commented Aug 17, 2023

@nwiltsie it seems it doesn't work :/

maybe add a "\" ?

@nwiltsie
Copy link
Contributor Author

Ah, what I had was valid R code that looked like badly-escaped python code. I got rid of the not-actually-an-escape-sequence, so that should fix it.

@nwiltsie nwiltsie temporarily deployed to dev August 17, 2023 21:35 — with GitHub Actions Inactive
@nwiltsie nwiltsie temporarily deployed to dev August 17, 2023 21:35 — with GitHub Actions Inactive
@nwiltsie
Copy link
Contributor Author

Judging by the logs, I think the issue is that Megalinter is using an outdated regex to detect if lintr found any issues with the file. There were lints, but the test falsely believes that there were not...

MegaLinter flavor is "all", no need to check match with linters

+----MATCHING LINTERS-+-----------------+----------------+------------+
| Descriptor | Linter | Criteria        | Matching files | Format/Fix |
+------------+--------+-----------------+----------------+------------+
| R          | lintr  | .r|.R|.Rmd|.RMD | 1              | no         |
+------------+--------+-----------------+----------------+------------+
::endgroup::
[lintr] CWD: /tmp/lint/.automation/test/r
[lintr] result: 0 Warning message:
Function with_defaults was deprecated in lintr version 3.0.0. Use linters_with_defaults or modify_defaults instead.
::warning file=/tmp/lint/.automation/test/r/r_bad_1.r,line=8,col=3::file=/tmp/lint/.automation/test/r/r_bad_1.r,line=8,col=3,[assignment_linter] Use <-, not =, for assignment.

...

::warning file=/tmp/lint/.automation/test/r/r_bad_1.r,line=43,col=1::file=/tmp/lint/.automation/test/r/r_bad_1.r,line=43,col=1,[trailing_blank_lines_linter] Trailing blank lines are superfluous.

[Text Reporter] Generated TEXT report: /tmp/0cc005a6-86ad-484f-b16a-c6e4929077b7/linters_logs/SUCCESS-R_LINTR.log
::group::✅ Linted [R] files with [lintr] successfully - (1.5s) (expand for details)
- Using [lintr v0.0.0] https://megalinter.io/test-nwiltsie-merge/descriptors/r_lintr
- MegaLinter key: [R_LINTR]
- Rules config: [/.automation/test/r/.lintr]
- Number of files analyzed: [1]
::endgroup::
[Post] No commands declared in user configuration
[Azure Comment Reporter] No Azure Token found, so skipped post of PR comment
[Gitlab Comment Reporter] No Gitlab Token found, so skipped post of MR comment

+----SUMMARY-+--------+------+-------+-------+--------+--------------+
| Descriptor | Linter | Mode | Files | Fixed | Errors | Elapsed time |
+------------+--------+------+-------+-------+--------+--------------+
| ✅ R       | lintr  | file |     1 |       |      0 |         1.5s |
+------------+--------+------+-------+-------+--------+--------------+

✅ Successfully linted all files without errors
Cleared MegaLinter runtime config for request bcb650b8-3d4c-11ee-b75b-0242ac110004

I think there's a mismatch between lintr's exit code and the test's expectation. lintr is saying that there are 0 errors and thus failing the test.

In main lintr is crashing, which produces the expected error, so the test incorrectly "passes". I suspect if we added a test case with style-only lints but that shouldn't produce any errors, main would start correctly failing.

@nvuillam
Copy link
Member

Judging by the logs, I think the issue is that Megalinter is using an outdated regex to detect if lintr found any issues with the file. There were lints, but the test falsely believes that there were not...

MegaLinter flavor is "all", no need to check match with linters

+----MATCHING LINTERS-+-----------------+----------------+------------+
| Descriptor | Linter | Criteria        | Matching files | Format/Fix |
+------------+--------+-----------------+----------------+------------+
| R          | lintr  | .r|.R|.Rmd|.RMD | 1              | no         |
+------------+--------+-----------------+----------------+------------+
::endgroup::
[lintr] CWD: /tmp/lint/.automation/test/r
[lintr] result: 0 Warning message:
Function with_defaults was deprecated in lintr version 3.0.0. Use linters_with_defaults or modify_defaults instead.
::warning file=/tmp/lint/.automation/test/r/r_bad_1.r,line=8,col=3::file=/tmp/lint/.automation/test/r/r_bad_1.r,line=8,col=3,[assignment_linter] Use <-, not =, for assignment.

...

::warning file=/tmp/lint/.automation/test/r/r_bad_1.r,line=43,col=1::file=/tmp/lint/.automation/test/r/r_bad_1.r,line=43,col=1,[trailing_blank_lines_linter] Trailing blank lines are superfluous.

[Text Reporter] Generated TEXT report: /tmp/0cc005a6-86ad-484f-b16a-c6e4929077b7/linters_logs/SUCCESS-R_LINTR.log
::group::✅ Linted [R] files with [lintr] successfully - (1.5s) (expand for details)
- Using [lintr v0.0.0] https://megalinter.io/test-nwiltsie-merge/descriptors/r_lintr
- MegaLinter key: [R_LINTR]
- Rules config: [/.automation/test/r/.lintr]
- Number of files analyzed: [1]
::endgroup::
[Post] No commands declared in user configuration
[Azure Comment Reporter] No Azure Token found, so skipped post of PR comment
[Gitlab Comment Reporter] No Gitlab Token found, so skipped post of MR comment

+----SUMMARY-+--------+------+-------+-------+--------+--------------+
| Descriptor | Linter | Mode | Files | Fixed | Errors | Elapsed time |
+------------+--------+------+-------+-------+--------+--------------+
| ✅ R       | lintr  | file |     1 |       |      0 |         1.5s |
+------------+--------+------+-------+-------+--------+--------------+

✅ Successfully linted all files without errors
Cleared MegaLinter runtime config for request bcb650b8-3d4c-11ee-b75b-0242ac110004

I think there's a mismatch between lintr's exit code and the test's expectation. lintr is saying that there are 0 errors and thus failing the test.

In main lintr is crashing, which produces the expected error, so the test incorrectly "passes". I suspect if we added a test case with style-only lints but that shouldn't produce any errors, main would start correctly failing.

@nwiltsie as I have no idea about how to use R, would you like to try to implement what you suggest ? :)

@nwiltsie
Copy link
Contributor Author

Hey @nvuillam, I unfortunately don't really have the time to dig deeper than this. I'll close this PR - as it's still linked to the original issue hopefully someone else can pick this up and resolve the test issues.

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

Successfully merging this pull request may close these issues.

lintr crashes with any lints
2 participants