Skip to content

Conversation

@BryanCutler
Copy link
Member

@BryanCutler BryanCutler commented Sep 5, 2018

This change enables checkstyle line length set at 120 columns and indentation checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this specific style of indentation is enforced by our checkstyle, but wanted to bring it up in case others had different preferences.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer this format. FYI I believe intellij does a different one where arguments hang off the opening parenthesis but I much prefer this pattern. +1 on enforcing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'll try to see if I can figure out how to enforce it. It's hard to tell right now because there are so many other warnings.

@jacques-n
Copy link
Contributor

I vote for 140 or 160. 100 is so short on these widescreens.

@BryanCutler
Copy link
Member Author

cc @icexelloss @jacques-n

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remind me why are we suppressing checks here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm suppressing all but the ones I have fixed so far, so I can do this incrementally. Eventually, this will be removed when the build passes all checks.

@BryanCutler
Copy link
Member Author

I vote for 140 or 160. 100 is so short on these widescreens.

I'm fine with going above 100, although having it too long makes it hard to read. I would probably prefer 120, but I'm ok with 140. So +1 for 140. Any opinion on this @icexelloss ?

@icexelloss
Copy link
Contributor

I am +1 for 120 but ok with 140 too.

@wesm
Copy link
Member

wesm commented Sep 5, 2018

FWIW In C++ we are 90 chars (Google limits to 80). But I guess Java devs mostly look at one file at a time? =)

@wesm
Copy link
Member

wesm commented Sep 5, 2018

@BryanCutler
Copy link
Member Author

@jacques-n would you be ok with trying out 120 for the line length? It's always easier to make it bigger if it becomes too cumbersome

@jacques-n
Copy link
Contributor

I'm okay with 120.

@BryanCutler BryanCutler changed the title [WIP] ARROW-3171: [Java] Enable checkstyle for line length [WIP] ARROW-3171: [Java] Enable checkstyle for line length and indentation Sep 5, 2018
@BryanCutler BryanCutler force-pushed the java-checkstyle-line-length-whitespace-ARROW-3171 branch from c8d16f2 to 59396db Compare September 5, 2018 23:21
@BryanCutler BryanCutler changed the title [WIP] ARROW-3171: [Java] Enable checkstyle for line length and indentation ARROW-3171: [Java] Enable checkstyle for line length and indentation Sep 5, 2018
@BryanCutler
Copy link
Member Author

Ok, I'm done with line length at 120 and indentation checks, please take another look - thanks!

@codecov-io
Copy link

codecov-io commented Sep 6, 2018

Codecov Report

Merging #2512 into master will increase coverage by 1.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2512      +/-   ##
==========================================
+ Coverage   86.33%   87.49%   +1.15%     
==========================================
  Files         308      259      -49     
  Lines       47120    44797    -2323     
==========================================
- Hits        40682    39194    -1488     
+ Misses       6366     5603     -763     
+ Partials       72        0      -72
Impacted Files Coverage Δ
go/arrow/datatype_nested.go
go/arrow/math/uint64_amd64.go
go/arrow/internal/testing/tools/bool.go
go/arrow/internal/bitutil/bitutil.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/array/null.go
go/arrow/math/uint64_avx2_amd64.go
go/arrow/array/builder.go
go/arrow/array/binary.go
go/arrow/array/numeric.gen.go
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21ef18b...59396db. Read the comment docs.

@BryanCutler
Copy link
Member Author

ping @icexelloss @jacques-n , look ok to merge?

@jacques-n
Copy link
Contributor

lgtm +1

@BryanCutler
Copy link
Member Author

merged to master, thanks @jacques-n and @icexelloss !

@BryanCutler BryanCutler deleted the java-checkstyle-line-length-whitespace-ARROW-3171 branch September 7, 2018 23:25
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
This change enables checkstyle line length set at 120 columns and indentation checks.

Author: Bryan Cutler <cutlerb@gmail.com>

Closes apache#2512 from BryanCutler/java-checkstyle-line-length-whitespace-ARROW-3171 and squashes the following commits:

59396db <Bryan Cutler> fixed indentation for arrow-plasma
ada5b0f <Bryan Cutler> fixed indentation for arrow-jdbc
d7277bd <Bryan Cutler> fixed indentation for arrow-tools
989bdbb <Bryan Cutler> fixed indentation for arrow-vector
41379b2 <Bryan Cutler> fixed indentation for arrow-memory
27a4f15 <Bryan Cutler> fixed line length for arrow-jdbc
4d9ca04 <Bryan Cutler> fixed line length for arrow-vector
9fab019 <Bryan Cutler> fixed line length for arrow-memory
4f137dc <Bryan Cutler> increase line length to 120
49085ad <Bryan Cutler> enabled line length checks
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.

5 participants