You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "baumgold (via GitHub)" <gi...@apache.org> on 2023/06/12 17:41:00 UTC
[GitHub] [arrow-julia] baumgold opened a new pull request, #464: Formatting
baumgold opened a new pull request, #464:
URL: https://github.com/apache/arrow-julia/pull/464
I used the default JuliaFormatter options, but we can customize them if we want to with a `.JuliaFormatter.toml` file. Details available here:
https://domluna.github.io/JuliaFormatter.jl
Fixes #398. CC: @svilupp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #464: Formatting
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1590170989
@quinnj / @ericphanson - shall I merge? Any objections?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on a diff in pull request #464: Formatting
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on code in PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#discussion_r1227155653
##########
src/FlatBuffers/builder.jl:
##########
@@ -63,8 +71,8 @@ end
Base.write(sink::Builder, o, x::Union{Bool,UInt8}) = sink.bytes[o+1] = UInt8(x)
function Base.write(sink::Builder, off, x::T) where {T}
off += 1
- for (i, ind) = enumerate(off:(off + sizeof(T) - 1))
- sink.bytes[ind] = (x >> ((i-1) * 8)) % UInt8
+ for (i, ind) in enumerate(off:(off+sizeof(T)-1))
+ sink.bytes[ind] = (x >> ((i - 1) * 8)) % UInt8
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #464: Formatting
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1588239678
> 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 `<:` operator in type parameter clauses, but I don't feel as strongly about that if people prefer otherwise.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #464: Formatting
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1589283690
@ericphanson - That script you speak of is embedded in the ci.yml file:
https://github.com/apache/arrow-julia/pull/464/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR165-R179
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on a diff in pull request #464: Formatting
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on code in PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#discussion_r1227153769
##########
src/FlatBuffers/FlatBuffers.jl:
##########
@@ -24,10 +24,10 @@ const VOffsetT = UInt16
const VtableMetadataFields = 2
basetype(::Enum) = UInt8
-basetype(::Type{T}) where {T <: Enum{S}} where {S} = S
+basetype(::Type{T}) where {T<:Enum{S}} where {S} = S
function readbuffer(t::AbstractVector{UInt8}, pos::Integer, ::Type{Bool})
- @inbounds b = t[pos + 1]
+ @inbounds b = t[pos+1]
Review Comment:
I always prefer spaces around operators
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #464: Formatting
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1588241793
> @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 think the CI change can be simplified using the "julia-format" GitHub action. I'll try to get this setup shortly.
https://github.com/julia-actions/julia-format
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] ericphanson commented on pull request #464: Formatting
Posted by "ericphanson (via GitHub)" <gi...@apache.org>.
ericphanson commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1589241678
Something I often do at work is check in a `format` directory with a Project and Manifest, a small `run.jl` script that does the formatting and a readme to explain how. Then the CI job can use the same project/manifest to do the check. This can allow folks to easily run the formatting locally and also ensure that JuliaFormatter updates don’t suddenly cause the CI check to fail on unrelated PRs. (Then if eg a new contributor’s PR fails the format check there is an easy remedy for them - to run the local run script in the format project).
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold merged pull request #464: Formatting
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold merged PR #464:
URL: https://github.com/apache/arrow-julia/pull/464
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on pull request #464: Formatting
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1587977525
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 `<:` operator in type parameter clauses, but I don't feel as strongly about that if people prefer otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #464: Formatting
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1588257717
I was trying to using the `julia-format` GitHub action but I can't seem to get it to work. Instead I took inspiration from the following example but simplified it as much as possible for use here.
https://github.com/julia-actions/julia-format/blob/master/workflows/format_check.yml
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on a diff in pull request #464: Formatting
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on code in PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#discussion_r1227157494
##########
src/FlatBuffers/table.jl:
##########
@@ -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))
Review Comment:
Maybe it's range-specific where it prefers to remove spaces?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on pull request #464: Formatting
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1590175688
Go for it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] codecov-commenter commented on pull request #464: Formatting
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1587814363
## [Codecov](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#464](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (509d035) into [main](https://app.codecov.io/gh/apache/arrow-julia/commit/5532270a27ce33ce98e640f28a8f597680786042?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5532270) will **decrease** coverage by `0.14%`.
> The diff coverage is `86.52%`.
```diff
@@ Coverage Diff @@
## main #464 +/- ##
==========================================
- Coverage 87.53% 87.39% -0.14%
==========================================
Files 26 26
Lines 3272 3276 +4
==========================================
- Hits 2864 2863 -1
- Misses 408 413 +5
```
| [Impacted Files](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [src/metadata/Flatbuf.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21ldGFkYXRhL0ZsYXRidWYuamw=) | `100.00% <ø> (ø)` | |
| [src/FlatBuffers/table.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL0ZsYXRCdWZmZXJzL3RhYmxlLmps) | `60.00% <61.53%> (ø)` | |
| [src/utils.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL3V0aWxzLmps) | `81.08% <66.66%> (ø)` | |
| [src/arraytypes/dictencoding.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL2FycmF5dHlwZXMvZGljdGVuY29kaW5nLmps) | `75.58% <74.19%> (ø)` | |
| [src/Arrow.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL0Fycm93Lmps) | `94.87% <75.00%> (-2.57%)` | :arrow_down: |
| [src/arraytypes/bool.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL2FycmF5dHlwZXMvYm9vbC5qbA==) | `88.70% <75.00%> (ø)` | |
| [src/arraytypes/fixedsizelist.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL2FycmF5dHlwZXMvZml4ZWRzaXplbGlzdC5qbA==) | `84.53% <77.77%> (ø)` | |
| [src/arraytypes/arraytypes.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL2FycmF5dHlwZXMvYXJyYXl0eXBlcy5qbA==) | `90.00% <78.94%> (ø)` | |
| [src/ArrowTypes/src/ArrowTypes.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL0Fycm93VHlwZXMvc3JjL0Fycm93VHlwZXMuamw=) | `86.85% <80.95%> (ø)` | |
| [src/eltypes.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL2VsdHlwZXMuamw=) | `85.76% <82.60%> (-0.30%)` | :arrow_down: |
| ... and [16 more](https://app.codecov.io/gh/apache/arrow-julia/pull/464?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on a diff in pull request #464: Formatting
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on code in PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#discussion_r1227332396
##########
src/FlatBuffers/FlatBuffers.jl:
##########
@@ -24,10 +24,10 @@ const VOffsetT = UInt16
const VtableMetadataFields = 2
basetype(::Enum) = UInt8
-basetype(::Type{T}) where {T <: Enum{S}} where {S} = S
+basetype(::Type{T}) where {T<:Enum{S}} where {S} = S
function readbuffer(t::AbstractVector{UInt8}, pos::Integer, ::Type{Bool})
- @inbounds b = t[pos + 1]
+ @inbounds b = t[pos+1]
Review Comment:
I added a `.JuliaFormatter.toml` file and set `whitespace_ops_in_indices = true` to accomplish this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on pull request #464: Formatting
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on PR #464:
URL: https://github.com/apache/arrow-julia/pull/464#issuecomment-1587978218
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org