Formatting #464
Formatting #464
Conversation
Codecov Report
@@ Coverage Diff @@
## main #464 +/- ##
==========================================
- Coverage 87.53% 4.93% -82.60%
==========================================
Files 26 25 -1
Lines 3272 3199 -73
==========================================
- Hits 2864 158 -2706
- Misses 408 3041 +2633
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/FlatBuffers/builder.jl
Outdated
| for (i, ind) in enumerate(off:(off+sizeof(T)-1)) | ||
| sink.bytes[ind] = (x >> ((i - 1) * 8)) % UInt8 |
There was a problem hiding this comment.
What the? what's the rule here? it seems to usually take away spaces in the 1st line, but adds them in the 2nd?
src/FlatBuffers/table.jl
Outdated
| @@ -72,7 +76,7 @@ function bytevector(t::Table, off) | |||
| off += get(t, off, UOffsetT) | |||
| start = off + sizeof(UOffsetT) | |||
| len = get(t, off, UOffsetT) | |||
| return view(bytes(t), (start + 1):(start + len + 1)) | |||
| return view(bytes(t), (start+1):(start+len+1)) | |||
There was a problem hiding this comment.
Maybe it's range-specific where it prefers to remove spaces?
|
I think this is mostly great and I agree we should do it. I think the only change I'm really against is removing spaces around operators, regardless of where or what kind of clause they occur in. I also personally prefer keeping spaces around the |
|
@baumgold, can you clarify the CI change here? Does it automatically add a commit w/ formatting changes to your PR? Or post suggestions that can be committed? |
I believe the only control for this that we are given in JuliaFormatter is the following: https://github.com/domluna/JuliaFormatter.jl#whitespace_ops_in_indices I overrode the default from false to true. That seems like it should mostly fix this issue. |
I think the CI change can be simplified using the "julia-format" GitHub action. I'll try to get this setup shortly. |
|
I was trying to using the https://github.com/julia-actions/julia-format/blob/master/workflows/format_check.yml The expected behavior is that if a PR is opened with malformed code then the format build will fail indicating that the PR should not be merged until the formatting is fixed. |
|
Something I often do at work is check in a One needs to manually PR the format manifest to update it for newer JuliaFormatter versions but imo that’s worth it being explicit (and you only need to update if there’s some specific change you want). This may be less necessary as JuliaFormatter has gotten more stable over the years. |
|
@ericphanson - That script you speak of is embedded in the ci.yml file: IMHO it's quite a trivial script. We can explicitly pin to a version of JuliaFormatter if we want but that adds extra maintenance overhead so would prefer not to unless there's a good reason. What do you think? Is this sufficient? |
|
@quinnj / @ericphanson - shall I merge? Any objections? |
|
Go for it |
I used the default JuliaFormatter options, but we can customize them if we want to with a
.JuliaFormatter.tomlfile. Details available here:https://domluna.github.io/JuliaFormatter.jl
Fixes #398. CC: @svilupp