You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tscottcoombes1 (via GitHub)" <gi...@apache.org> on 2024/05/30 08:24:51 UTC

[PR] Go pre commit [arrow]

tscottcoombes1 opened a new pull request, #41888:
URL: https://github.com/apache/arrow/pull/41888

   ### Rationale for this change
   Add and run pre-commit 
   
   ### What changes are included in this PR?
   Update pre-commit file
   Run pre-commit
   
   ### Are these changes tested?
   N/A
   
   ### Are there any user-facing changes?
   No
   
   GitHub Issue: https://github.com/apache/arrow/issues/41887


-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "tscottcoombes1 (via GitHub)" <gi...@apache.org>.
tscottcoombes1 commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1622837948


##########
.gitignore:
##########
@@ -102,4 +102,9 @@ __debug_bin
 .envrc
 
 # Develocity
+.mvn/.gradle-enterprise/
 .mvn/.develocity/
+
+# rat
+filtered_rat.txt
+rat.txt

Review Comment:
   They get created by the `release audit tool` when I run pre-commit 
   As they're not part of the project, I assume I'm not meant to commit them, so i ignored them



-- 
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

Re: [PR] Go pre commit [arrow]

Posted by "tscottcoombes1 (via GitHub)" <gi...@apache.org>.
tscottcoombes1 commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1620245612


##########
go/arrow/internal/debug/assert_off.go:
##########
@@ -14,6 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build !assert

Review Comment:
   I don't know too much about these, https://stackoverflow.com/questions/68360688/whats-the-difference-between-gobuild-and-build-directives
   
   I'm happy to run go-fix as well



-- 
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

Re: [PR] Go pre commit [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41888:
URL: https://github.com/apache/arrow/pull/41888#issuecomment-2138996564

   <!--
     Licensed to the Apache Software Foundation (ASF) under one
     or more contributor license agreements.  See the NOTICE file
     distributed with this work for additional information
     regarding copyright ownership.  The ASF licenses this file
     to you under the Apache License, Version 2.0 (the
     "License"); you may not use this file except in compliance
     with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
     Unless required by applicable law or agreed to in writing,
     software distributed under the License is distributed on an
     "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
     KIND, either express or implied.  See the License for the
     specific language governing permissions and limitations
     under the License.
   -->
   
   Thanks for opening a pull request!
   
   If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
   
   Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
   
   Then could you also rename the pull request title in the following format?
   
       GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
   
   or
   
       MINOR: [${COMPONENT}] ${SUMMARY}
   
   In the case of PARQUET issues on JIRA the title also supports:
   
       PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
   
   See also:
   
     * [Other pull requests](https://github.com/apache/arrow/pulls/)
     * [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
   


-- 
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

Re: [PR] GH-41887: [Go] pre commit [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1620898898


##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,8 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/dnephin/pre-commit-golang
+    rev: v0.5.1
+    hooks:
+      - id: go-fmt
+      - id: go-imports

Review Comment:
   arrow-adbc uses [golangci-lint](https://github.com/apache/arrow-adbc/blob/main/.pre-commit-config.yaml#L61), should we just use that instead? 



-- 
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

Re: [PR] GH-41887: [Go] pre commit [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41888:
URL: https://github.com/apache/arrow/pull/41888#issuecomment-2139006802

   :warning: GitHub issue #41887 **has been automatically assigned in GitHub** to PR creator.


-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1622844623


##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,8 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/dnephin/pre-commit-golang
+    rev: v0.5.1
+    hooks:
+      - id: go-fmt
+      - id: go-imports

Review Comment:
   Fair, we can do that as a follow-up



-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1622978356


##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,8 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/dnephin/pre-commit-golang

Review Comment:
   It seems that this project isn't maintained: https://github.com/dnephin/pre-commit-golang/issues/98
   
   Can we use other repo?



-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1623183018


##########
.github/workflows/dev.yml:
##########
@@ -49,6 +49,10 @@ jobs:
         uses: actions/setup-python@82c7e631bb3cdc910f68e0081d67478d79c6982d # v5.1.0
         with:
           python-version: 3.12
+      - name: Install pre-commit dependencies
+        run: |
+          go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.59.0
+      - run: echo "PATH=$PATH:/home/runner/go/bin" >> $GITHUB_ENV

Review Comment:
   Can we remove this?
   It seems that `pre-commit run` installs `golangci-lint` automatically.



##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,20 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/golangci/golangci-lint
+    rev: v1.59.0
+    hooks:
+      # no built-in support for multiple go.mod
+      # https://github.com/golangci/golangci-lint/issues/828
+      - id: golangci-lint
+        name: golangci-lint-arrow
+        entry: bash -c 'cd go/arrow && golangci-lint run --fix --disable-all -E goimports -E gofmt'

Review Comment:
   Can we create a common configuration file instead of specifying `--disable*`/`--enable*`?



##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,20 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/golangci/golangci-lint
+    rev: v1.59.0
+    hooks:
+      # no built-in support for multiple go.mod
+      # https://github.com/golangci/golangci-lint/issues/828
+      - id: golangci-lint
+        name: golangci-lint-arrow
+        entry: bash -c 'cd go/arrow && golangci-lint run --fix --disable-all -E goimports -E gofmt'
+        types_or: [ go ]

Review Comment:
   Can we remove this? It seems that it's the default: https://github.com/golangci/golangci-lint/blob/master/.pre-commit-hooks.yaml#L5
   
   ```suggestion
   ```



##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,20 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/golangci/golangci-lint
+    rev: v1.59.0
+    hooks:
+      # no built-in support for multiple go.mod
+      # https://github.com/golangci/golangci-lint/issues/828
+      - id: golangci-lint
+        name: golangci-lint-arrow

Review Comment:
   How about using `golangci-lint-full` because `golangci-lint run --fix` is the entry used by `golangci-lint-full`?
   https://github.com/golangci/golangci-lint/blob/master/.pre-commit-hooks.yaml#L12
   
   ```suggestion
         - id: golangci-lint-full
           name: golangci-lint-full-arrow
   ```



-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "tscottcoombes1 (via GitHub)" <gi...@apache.org>.
tscottcoombes1 commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1623027020


##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,8 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/dnephin/pre-commit-golang

Review Comment:
   yes, changed to golangci-lint to match arrow-adbc, but disabled most options



-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #41888:
URL: https://github.com/apache/arrow/pull/41888#issuecomment-2142825114

   It looks like the `pre-commit` isn't installing `goimports` you may need to update the CI yaml to manually install 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

Re: [PR] GH-41887: [Go] pre commit [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1620882778


##########
go/arrow/internal/debug/assert_off.go:
##########
@@ -14,6 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build !assert

Review Comment:
   This is fine, it's because the format changed (in a backwards compatible way) around go 1.18 or so. `//go:build` is the correct current one for the latest versions of the Go toolchain



-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1623030095


##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,8 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/dnephin/pre-commit-golang

Review Comment:
   Oh nice catch!



-- 
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

Re: [PR] GH-41887: [Go] pre commit [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1620886345


##########
.gitignore:
##########
@@ -102,4 +102,9 @@ __debug_bin
 .envrc
 
 # Develocity
+.mvn/.gradle-enterprise/
 .mvn/.develocity/
+
+# rat
+filtered_rat.txt
+rat.txt

Review Comment:
   Why are these being added?



-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "tscottcoombes1 (via GitHub)" <gi...@apache.org>.
tscottcoombes1 commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1622836170


##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,8 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/dnephin/pre-commit-golang
+    rev: v0.5.1
+    hooks:
+      - id: go-fmt
+      - id: go-imports

Review Comment:
   We could add that as well, but then I would need to fix all the issues it complains about. 



-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #41888:
URL: https://github.com/apache/arrow/pull/41888


-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41888:
URL: https://github.com/apache/arrow/pull/41888#discussion_r1623338074


##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,17 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/golangci/golangci-lint
+    rev: v1.59.0
+    hooks:
+      # no built-in support for multiple go.mod
+      # https://github.com/golangci/golangci-lint/issues/828
+      - id: golangci-lint
+        name: golangci-lint-full-arrow
+        entry: bash -c 'cd go/arrow && golangci-lint run'
+      - id: golangci-lint
+        name: golangci-lint-full-parquet
+        entry: bash -c 'cd go/parquet && golangci-lint run'
+      - id: golangci-lint

Review Comment:
   ```suggestion
         - id: golangci-lint-full
   ```



##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,17 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/golangci/golangci-lint
+    rev: v1.59.0
+    hooks:
+      # no built-in support for multiple go.mod
+      # https://github.com/golangci/golangci-lint/issues/828
+      - id: golangci-lint
+        name: golangci-lint-full-arrow
+        entry: bash -c 'cd go/arrow && golangci-lint run'
+      - id: golangci-lint

Review Comment:
   ```suggestion
         - id: golangci-lint-full
   ```



##########
.golangci.yaml:
##########


Review Comment:
   Could you add our license header to this file?



##########
.pre-commit-config.yaml:
##########
@@ -141,3 +141,17 @@ repos:
           '--disable',
           'dangling-hyphen,line-too-long',
         ]
+  - repo: https://github.com/golangci/golangci-lint
+    rev: v1.59.0
+    hooks:
+      # no built-in support for multiple go.mod
+      # https://github.com/golangci/golangci-lint/issues/828
+      - id: golangci-lint

Review Comment:
   ```suggestion
         - id: golangci-lint-full
   ```



-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41888:
URL: https://github.com/apache/arrow/pull/41888#issuecomment-3638598024

   After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 99014abd193c84aee7490a36fd7914389b84cfcd.
   
   None of the specified runs were found on the Conbench server.
   
   The [full Conbench report](https://github.com/apache/arrow/runs/57706139871) has more details.


-- 
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

Re: [PR] GH-41887: [Go] Run linter via pre-commit [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41888:
URL: https://github.com/apache/arrow/pull/41888#issuecomment-3649547473

   After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 99014abd193c84aee7490a36fd7914389b84cfcd.
   
   None of the specified runs were found on the Conbench server.
   
   The [full Conbench report](https://github.com/apache/arrow/runs/57975750583) has more details.


-- 
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