thisisnic commented on code in PR #50375: URL: https://github.com/apache/arrow/pull/50375#discussion_r4458450373
########## AGENTS.md: ########## @@ -0,0 +1,219 @@ +<!--- + 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. +--> + +# AGENTS.md + +Guidance for AI coding agents working on Apache Arrow. It covers the +conventions shared across the repository; component directories may +carry their own `AGENTS.md` with build and test commands, and the +closest file to the code you are editing takes precedence. + +## Repository Gotchas + +- This is a monorepo. The C++ library (`cpp/`) is the shared core; + PyArrow (`python/`), R (`r/`), and MATLAB (`matlab/`) bind to it + directly, C/GLib (`c_glib/`) wraps it, and Ruby (`ruby/`) binds to + C/GLib. A change in `cpp/` can break any of these, so check the + dependent bindings when you touch C++. +- Java, Go, Rust, C#, JavaScript, Swift, and Julia are **not** in this + repository. They live in `apache/arrow-java`, `apache/arrow-go`, + `apache/arrow-rs`, `apache/arrow-dotnet`, `apache/arrow-js`, + `apache/arrow-swift`, and `apache/arrow-julia`. Do not look for or + recreate them here. +- Generated files are committed and must not be hand-edited: + `r/R/arrowExports.R` and `r/src/arrowExports.cpp` (regenerated by + `r/data-raw/codegen.R`), and everything under `cpp/src/generated/`. +- C++ has two build systems. A file added to or removed from a + `CMakeLists.txt` usually needs the same change in the sibling + `meson.build`. +- Every new source file needs the Apache License 2.0 header. Copy it + from an existing file in the same directory. CI's license check + (`rat`) rejects files without one. + +## Building and Testing + +Each component has its own toolchain. Read the developer docs for the +component before building, rather than guessing: + +- C++: `docs/source/developers/cpp/building.rst` +- Python: `docs/source/developers/python/building.rst` +- R: `r/vignettes/developers/setup.Rmd` +- Everything else: `docs/source/developers/` +- Running tests, per component: + `docs/source/developers/guide/step_by_step/testing.rst` + +Build and run the affected tests locally before pushing. CI is not a +substitute for a component's unit tests: it is slow and shared, and a +test that was never compiled is the first thing a reviewer sees. If +you cannot build the component, enable GitHub Actions on your fork +and don't open the PR until the jobs your change touches pass there. + +Run pre-commit on the files you changed: + +```bash +pip install pre-commit +pre-commit install +pre-commit run --files <paths you changed> +pre-commit run --all-files r # one component; aliases are listed in .pre-commit-config.yaml +pre-commit run -a # every hook +``` + +### Writing Tests + +- One test per behaviour. Fold near-identical cases into one test + with a loop or `parametrize` rather than copying the function. +- A regression test must fail without the fix. Check this before + pushing. +- Don't duplicate C++ test coverage in Python or R; binding tests + only need to show the plumbing works. +- Don't test compute kernels on invalid arrays (they are only + specified for valid input), and don't test type variants that share + a code path. Errors the code raises on purpose still need a test. +- Call `ValidateFull()` on arrays a C++ test builds. + +## Writing Code + +- Fix what the issue describes, nothing more. Don't generalise, rename, + or reformat nearby code, and keep unrelated files out of the diff. +- Internal preconditions are documented (or `DCHECK`ed), not validated + with error paths. Reviewers reject input validation nobody asked for. +- Before writing a bitmap loop, a visitor, an index normaliser, or a + test helper, look for an existing one in `cpp/src/arrow/util/` and + the nearest `testing/` or `test_util` file. There usually is one. +- Comments describe the code for a future reader, never the PR, the + bug, or what changed. If a comment says "this change", "now", + "still", or "previously", delete it or move it to the PR description. + A bare `// GH-12345` above a regression test is fine. +- Performance claims need numbers. For C++, run + `archery benchmark diff --benchmark-filter=<name> HEAD upstream/main` + (see `docs/source/developers/benchmarks.rst`) and paste the table + into the PR description. Pass the baseline explicitly: the default + is `origin/HEAD`, which is the fork, not `apache/arrow`. A + maintainer can also trigger the CI benchmark run; note in the + description if that would help. + +## Git + +- `upstream` is `apache/arrow`; `origin` is the contributor's fork. + Fetch from `upstream`, push branches to `origin`, and check + `git remote -v` before pushing. Never push to `upstream`. +- Never open a PR from your fork's `main` branch. Fork-sync bots + overwrite it, which empties the PR. +- PRs are squash-merged, so the PR title and description become the + commit message. Spend effort there rather than on individual commits. +- Rebase on `upstream/main` when the branch is stale, and force-push + your own PR branch; reviewers expect that. An outdated branch makes + CI run every job and often fails for unrelated reasons. + +## Issues + +- Prefix the title with the component(s) in brackets: + `[C++][Python] Short description of the problem`. +- Do NOT include a `GH-` prefix in issue titles. That is for PR titles + only, where it refers to the issue being fixed. +- Search existing issues before creating one, and use the templates in + `.github/ISSUE_TEMPLATE/`. Review Comment: I don't think we should ban it outright - I use it for describing CI failures and think it does a good job of it. But perhaps for bugs and feature requests, we could get it to ask the user to describe how they were affected in their own words? What do poorly-crafted agent-created issues look like? I'm thinking too much information, complex descriptions; anything else, @pitrou? ########## AGENTS.md: ########## @@ -0,0 +1,219 @@ +<!--- + 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. +--> + +# AGENTS.md + +Guidance for AI coding agents working on Apache Arrow. It covers the +conventions shared across the repository; component directories may +carry their own `AGENTS.md` with build and test commands, and the +closest file to the code you are editing takes precedence. + +## Repository Gotchas + +- This is a monorepo. The C++ library (`cpp/`) is the shared core; + PyArrow (`python/`), R (`r/`), and MATLAB (`matlab/`) bind to it + directly, C/GLib (`c_glib/`) wraps it, and Ruby (`ruby/`) binds to + C/GLib. A change in `cpp/` can break any of these, so check the + dependent bindings when you touch C++. +- Java, Go, Rust, C#, JavaScript, Swift, and Julia are **not** in this + repository. They live in `apache/arrow-java`, `apache/arrow-go`, + `apache/arrow-rs`, `apache/arrow-dotnet`, `apache/arrow-js`, + `apache/arrow-swift`, and `apache/arrow-julia`. Do not look for or + recreate them here. +- Generated files are committed and must not be hand-edited: + `r/R/arrowExports.R` and `r/src/arrowExports.cpp` (regenerated by + `r/data-raw/codegen.R`), and everything under `cpp/src/generated/`. +- C++ has two build systems. A file added to or removed from a + `CMakeLists.txt` usually needs the same change in the sibling + `meson.build`. +- Every new source file needs the Apache License 2.0 header. Copy it + from an existing file in the same directory. CI's license check + (`rat`) rejects files without one. + +## Building and Testing + +Each component has its own toolchain. Read the developer docs for the +component before building, rather than guessing: + +- C++: `docs/source/developers/cpp/building.rst` +- Python: `docs/source/developers/python/building.rst` +- R: `r/vignettes/developers/setup.Rmd` +- Everything else: `docs/source/developers/` +- Running tests, per component: + `docs/source/developers/guide/step_by_step/testing.rst` + +Build and run the affected tests locally before pushing. CI is not a +substitute for a component's unit tests: it is slow and shared, and a +test that was never compiled is the first thing a reviewer sees. If +you cannot build the component, enable GitHub Actions on your fork +and don't open the PR until the jobs your change touches pass there. Review Comment: Oops, I'll change -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
