pitrou commented on code in PR #46530:
URL: https://github.com/apache/arrow/pull/46530#discussion_r2126837861
##########
dev/archery/archery/cli.py:
##########
@@ -745,6 +766,30 @@ def _set_default(opt, default):
@click.option('--with-rust', type=bool, default=False,
help='Include Rust in integration tests',
envvar="ARCHERY_INTEGRATION_WITH_RUST")
[email protected]('--with-external-library', type=click.Path(exists=True,
file_okay=False, dir_okay=True, executable=False),
+ help='Path to the external library to include in integration
tests',
+ envvar="ARCHERY_INTEGRATION_WITH_EXTERNAL_LIBRARY")
[email protected]('--external-library-ipc-producer', is_flag=True, default=False,
callback=validate_conditional_options,
Review Comment:
Can you ensure you wrap long lines to max 90 ~chars? This makes the code
more readable, especially in diff views.
##########
dev/archery/archery/cli.py:
##########
@@ -844,29 +889,21 @@ def integration(with_all=False, random_seed=12345,
**args):
implementations = ['cpp', 'csharp', 'java', 'js', 'go', 'nanoarrow',
'rust']
formats = ['ipc', 'flight', 'c_data']
- enabled_implementations = 0
- for lang in implementations:
- param = f'with_{lang}'
- if with_all:
- args[param] = with_all
- enabled_implementations += args[param]
-
- enabled_formats = 0
- for fmt in formats:
- param = f'run_{fmt}'
- enabled_formats += args[param]
-
if gen_path:
# XXX See GH-37575: this option is only used by the JS test suite
# and might not be useful anymore.
os.makedirs(gen_path, exist_ok=True)
write_js_test_json(gen_path)
else:
- if enabled_formats == 0:
+ have_enabled_formats: Final[bool] = any(args[f"run_{fmt}"] for fmt in
formats)
Review Comment:
The `Final` annotation seems pedantic to me. Besides, `any` returns a
boolean, so no annotation should be required.
##########
dev/archery/README.md:
##########
@@ -48,4 +48,90 @@ Additionally, if you would prefer to install everything at
once,
`pip install -e "arrow/dev/archery[all]"` is an alias for all of
the above subpackages.
-For some prior art on benchmarking in Arrow, see [this
prototype](https://github.com/apache/arrow/tree/0409498819332fc479f8df38babe3426d707fb9e/dev/benchmarking).
\ No newline at end of file
+For some prior art on benchmarking in Arrow, see [this
prototype](https://github.com/apache/arrow/tree/0409498819332fc479f8df38babe3426d707fb9e/dev/benchmarking).
+
+# Usage
+## Integration tests
Review Comment:
We already have docs for the Integration tests in
https://github.com/apache/arrow/blob/main/docs/source/format/Integration.rst, I
suggest augmenting this document rather than the README.
##########
dev/archery/archery/cli.py:
##########
@@ -844,29 +889,21 @@ def integration(with_all=False, random_seed=12345,
**args):
implementations = ['cpp', 'csharp', 'java', 'js', 'go', 'nanoarrow',
'rust']
formats = ['ipc', 'flight', 'c_data']
- enabled_implementations = 0
- for lang in implementations:
- param = f'with_{lang}'
- if with_all:
- args[param] = with_all
- enabled_implementations += args[param]
-
- enabled_formats = 0
- for fmt in formats:
- param = f'run_{fmt}'
- enabled_formats += args[param]
-
if gen_path:
# XXX See GH-37575: this option is only used by the JS test suite
# and might not be useful anymore.
os.makedirs(gen_path, exist_ok=True)
write_js_test_json(gen_path)
else:
- if enabled_formats == 0:
+ have_enabled_formats: Final[bool] = any(args[f"run_{fmt}"] for fmt in
formats)
+ if not have_enabled_formats:
raise click.UsageError(
"Need to enable at least one format to test "
"(IPC, Flight, C Data Interface); try --help")
- if enabled_implementations == 0:
+ have_enabled_implementation: Final[bool] = with_all or any(
Review Comment:
Same here.
--
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]
