Skip to content

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
chelseajonesr pushed a commit to chelseajonesr/arrow that referenced this pull request Jul 20, 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 <thisisnic@gmail.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
@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.

R-JunmingChen pushed a commit to R-JunmingChen/arrow that referenced this pull request Aug 20, 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 <thisisnic@gmail.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
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 <thisisnic@gmail.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
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