Nishuuzz opened a new pull request, #55162:
URL: https://github.com/apache/arrow/pull/55162
### Rationale for this change
`archery integration --with-all` accepts the flag and then ignores it, so it
enables nothing. The command our own docs give for running everything fails:
```console
$ archery integration --with-all --run-flight --run-ipc --run-c-data
Error: Need to enable at least one implementation to test; try --help
```
`with_all` is bound as an explicit parameter of `integration()`, which also
keeps it out of `**args`, so neither the body nor `select_testers()` ever sees
it. Every `--with-<lang>` option is declared `default=False`, so
`select_testers()` always gets an explicit `False` for each one and its own
`with_cpp=True, with_java=True, ...` defaults never get a chance to apply.
As far as I can tell this has never worked. ARROW-6466 (5ca85922ae) added
`--with-all`, declared the `--with-<lang>` options with `default=False`, and
added this helper immediately above the command:
```python
def _set_default(opt, default):
if opt is None:
return default
return opt
```
`_set_default` is not called anywhere in the repository. That reads to me
like `--with-all` was meant to supply the default for the individual options
and the wiring was never finished, so this PR finishes it rather than inventing
a new mechanism.
Closes #55161.
### What changes are included in this PR?
- The eight `--with-<lang>` options now use `default=None` instead of
`default=False`. That keeps "not given" distinguishable from an explicit
`--with-java=0`, which is what makes the precedence below possible.
- Before `select_testers()`, any `with_*` option still unset falls back to
the value of `--with-all`, via the existing `_set_default` helper.
Precedence ends up as: an explicitly given `--with-<lang>` (or its
`ARCHERY_INTEGRATION_WITH_*` environment variable) wins; otherwise `--with-all`
decides; otherwise `False`, as before. So `--with-all --with-java=0` means
"everything except Java", which matches the flag's help text of including
implementations *by default*.
I deliberately did not touch `docs/source/format/Integration.rst` — the
documented command is correct, it was the code that did not match it.
### Are these changes tested?
Yes, three tests in `archery/tests/test_cli.py` that patch `select_testers`
and assert on what it receives.
Verification I ran locally, since not all of it is visible from the diff:
- The two `--with-all` tests fail on unmodified `main` (`assert False is
True`) and pass with the change. The third, that *without* `--with-all` nothing
is enabled, passes both before and after on purpose — it is there to pin down
that the default behaviour is unchanged.
- Full `archery` test run before and after, excluding three modules that
need optional deps I don't have installed (`responses` and friends): 26 failed
/ 24 passed / 1 skipped before, 26 failed / 27 passed / 1 skipped after. The
same 26 failures are pre-existing `docker` `FileNotFoundError`s on this
machine; the delta is exactly the three new tests.
- Checked the resulting flags by hand through `CliRunner` for: no flags,
`--with-all`, the full documented command, `--with-all --with-java=0`, and
`--with-cpp=1`. On `main` all three `--with-all` cases enable nothing; with the
change they enable all eight, and `--with-cpp=1` is unaffected either way.
- `flake8 --config python/setup.cfg` clean on both changed files.
I could not run the integration tests themselves end to end, since I don't
have the other implementations built here, so I have not observed an actual
cross-language run under `--with-all`. What I verified is the flag plumbing up
to the `select_testers()` call.
### Are there any user-facing changes?
`archery integration --with-all` now does what its help text says. Nothing
changes for invocations that don't pass it: with no `--with-<lang>` and no
`--with-all`, every implementation is still disabled, as before.
### Was AI used for this PR?
In accordance to the [AI generation
guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code),
please disclose below whether and how AI was used in this PR.
The bug was found by a static scan for parameters that a function accepts
and never reads, which flagged `with_all`. The code, tests and this description
were AI-assisted, and the verification steps listed above were all actually run
rather than assumed. Happy to explain or change any part of it.
**PR code and description written by:**
- [ ] Human
- [x] AI
**Reviewed before submission by:**
- [ ] Human
- [x] AI
- [ ] Not reviewed
--
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]
