Skip to content

Comments

GH-36787: [R] lintr update leads to failing tests on main#36788

Merged
thisisnic merged 1 commit intoapache:mainfrom
thisisnic:GH-36787_lintr_failures
Jul 20, 2023
Merged

GH-36787: [R] lintr update leads to failing tests on main#36788
thisisnic merged 1 commit intoapache:mainfrom
thisisnic:GH-36787_lintr_failures

Conversation

@thisisnic
Copy link
Member

@thisisnic thisisnic commented Jul 20, 2023

What changes are included in this PR?

Turning off the newly introduced indentation linter as it causes test failures, and isn't in sync with styler which we use to style our code.

Are these changes tested?

No, is linter config.

Are there any user-facing changes?

No

@thisisnic thisisnic requested a review from paleolimbot as a code owner July 20, 2023 10:42
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Jul 20, 2023
@thisisnic thisisnic self-assigned this Jul 20, 2023
Copy link
Member

@paleolimbot paleolimbot 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 curious...is it worth fixing the indentation problems or are they spurious?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 20, 2023
@thisisnic
Copy link
Member Author

Thanks! Just curious...is it worth fixing the indentation problems or are they spurious?

Good question! I had a look at some of them, and they seem to be stylistic choices which don't make the code look better rather than fixing true problems. More importantly, using make style doesn't "correct" them to the new lintr standards, so it seems problematic to enable it right now.

@thisisnic thisisnic merged commit fca6a66 into apache:main Jul 20, 2023
@thisisnic thisisnic added this to the 14.0.0 milestone Jul 20, 2023
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit fca6a66.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…he#36788)

### What changes are included in this PR?

Turning off the newly introduced indentation linter as it causes test failures, and isn't in sync with styler which we use to style our code.

### Are these changes tested?

No, is linter config.

### Are there any user-facing changes?

No
* Closes: apache#36787

Authored-by: Nic Crane <[email protected]>
Signed-off-by: Nic Crane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting merge Awaiting merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[R] lintr update leads to failing tests on main

2 participants