You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/04 19:13:55 UTC
[GitHub] [arrow-datafusion] timvw opened a new pull request, #2444: Issue 2321: ehance documentation on how to format/check your code
timvw opened a new pull request, #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444
# Which issue does this PR close?
Closes #https://github.com/apache/arrow-datafusion/issues/2321
# Are there any user-facing changes?
No.
--
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-datafusion] tustvold commented on a diff in pull request #2444: Issue 2321: ehance documentation on how to format/check your code
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865592842
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
What are people's thoughts on merging the `lint`, `clippy` and `cargo-toml-formatting-checks` into a single `lint` check, moving this to script `ci/scripts/rust_lint.sh`, and running it from CI? This will ensure it remains up to date?
@alamb @andygrove ??
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
+
+# 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.
+set -e
+cargo fmt --all
Review Comment:
```suggestion
if ! command -v cargo-tomlfmt &> /dev/null; then
echo "Installing cargo-tomlfmt using cargo"
cargo install cargo-tomlfmt
fi
cargo fmt --all
```
--
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-datafusion] alamb commented on pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#issuecomment-1118588090
Thank you for driving this @timvw and @jdye64
--
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-datafusion] timvw commented on a diff in pull request #2444: Issue 2321: ehance documentation on how to format/check your code
Posted by GitBox <gi...@apache.org>.
timvw commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865759797
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
Personally, I appreciate the fact that multiple steps/commands are visualised as multiple steps/commands in the build pipeline.
The CI build and the dev/lint-code script could be updated to invoke the same scripts (run_clippy.sh, run_xxx, ..) but unless the commands and/or their arguments change often, I would not bother..
--
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-datafusion] timvw commented on a diff in pull request #2444: Issue 2321: ehance documentation on how to format/check your code
Posted by GitBox <gi...@apache.org>.
timvw commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865759797
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
Personally, I appreciate the fact that multiple steps/commands are visualised as multiple steps/commands in the build pipeline. It's not pleasant needing to dive into the error log to find out which step failed..
The CI build and the dev/lint-code script could be updated to invoke the same scripts (run_clippy.sh, run_xxx, ..) but unless the commands and/or their arguments change often, I would not bother..
--
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-datafusion] jdye64 commented on a diff in pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
jdye64 commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865859358
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
What about adding a `.pre-commit-config.yaml` file and a sentence or 2 about installing that as a local git hook to ensure that the steps are ran before each commit? This would allow for the user to opt in to running the pre-commits but once they do also help catch commits with unnoticed lint issues. Happy to add that as another PR on top of this PR.
This is something we use in [cuDF](https://github.com/rapidsai/cudf/blob/branch-22.06/CONTRIBUTING.md#python--pre-commit-hooks) this is python specific and is only to illustrate an example.
--
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-datafusion] tustvold commented on a diff in pull request #2444: Issue 2321: ehance documentation on how to format/check your code
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865598550
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
+
+# 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.
+set -e
+cargo fmt --all
+cargo clippy --all-targets --workspace -- -D warnings
+find . -mindepth 2 -name 'Cargo.toml' -exec cargo tomlfmt -p {} \; ; git diff --exit-code
Review Comment:
```suggestion
find . -mindepth 2 -name 'Cargo.toml' -exec cargo tomlfmt -p {} \;
```
--
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-datafusion] timvw commented on a diff in pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
timvw commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r866121467
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
Added branches to verify that build still fails when one of the checks is not ok:
* https://github.com/timvw/arrow-datafusion/tree/ISSUE-2321-break-fmt -> build caught issue: https://github.com/timvw/arrow-datafusion/actions/runs/2276886712
* https://github.com/timvw/arrow-datafusion/tree/ISSUE-2321-break-toml -> build caught issue:
https://github.com/timvw/arrow-datafusion/actions/runs/2276894979
* https://github.com/timvw/arrow-datafusion/tree/ISSUE-2321-break-clippy -> build caught issue:
https://github.com/timvw/arrow-datafusion/actions/runs/2276926769
--
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-datafusion] timvw commented on a diff in pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
timvw commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r866029017
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
Created the scripts
Updated rust.yml to call the scripts
Updated (and renamed) dev/rust_lint.sh to call the scripts
--
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-datafusion] tustvold commented on a diff in pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865776039
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
Fair enough
--
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-datafusion] alamb commented on a diff in pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865936702
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
I would be quite happy to help those who wanted to install a pre-commit hook but am opposed to anything that required it / made it the default in this repository as various developers have their own ideas of ideal workflows
I suggest adding the three scripts as suggested
```
ci/scripts/rust_fmt.sh
ci/scripts/rust_clippy.sh
ci/scripts/toml_fmt.sh
```
that are invoked from the CI jobs (and thus remain in sync)
And then people can mix/match however they want to run 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
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #2444: Issue 2321: ehance documentation on how to format/check your code
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865598550
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
+
+# 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.
+set -e
+cargo fmt --all
+cargo clippy --all-targets --workspace -- -D warnings
+find . -mindepth 2 -name 'Cargo.toml' -exec cargo tomlfmt -p {} \; ; git diff --exit-code
Review Comment:
```suggestion
find . -mindepth 2 -name 'Cargo.toml' -exec cargo tomlfmt -p {} \;
```
CI cares that the diff is clean, I'm not sure locally it should
--
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-datafusion] jdye64 commented on a diff in pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
jdye64 commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865894525
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
Good point. Why don't we just start with `cargo fmt` for now then. Does that sound reasonable? I feel like those are 80% of the issues that slip through the cracks on commits anyway.
--
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-datafusion] tustvold commented on a diff in pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444#discussion_r865879435
##########
dev/format-code.sh:
##########
@@ -0,0 +1,22 @@
+#!/bin/bash
Review Comment:
Definitely an option but you need to be careful about two things though:
* Staging only the files that are relevant - https://github.com/influxdata/influxdb_iox/pull/839/files#diff-87320095d7c4f39eed5d8f3866b512eb910e4eec8e7926faaeef6a53ab786fcbR8
* Avoiding the git hook taking too long
The latter is the really painful one because clippy is very slow, you get no output from git hooks, and they run on most git operations. Formatting could definitely be done in this way though
--
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-datafusion] andygrove merged pull request #2444: Issue 2321: Add code formatting instructions to CONTRIBUTING
Posted by GitBox <gi...@apache.org>.
andygrove merged PR #2444:
URL: https://github.com/apache/arrow-datafusion/pull/2444
--
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