laskoviymishka commented on code in PR #15:
URL:
https://github.com/apache/iceberg-verification/pull/15#discussion_r4247625998
##########
.prettierrc.json:
##########
@@ -0,0 +1,3 @@
+{
+ "printWidth": 120
Review Comment:
Prettier with only `printWidth` set doesn't pin one canonical shape — it
preserves whatever line break the author put between `{` and the first key, so
two different layouts of the same case both pass `--check`. The diff shows it:
some objects collapse to one line, others stay exploded. If the goal is
fixtures that stay readable as the list grows, I'd add `objectWrap: "collapse"`
(Prettier 3.5+) so layout is a pure function of content. If we're fine with
per-author layout, then `make lint` is really just an idempotence check and the
CONTRIBUTING wording should say so.
##########
Makefile:
##########
@@ -0,0 +1,81 @@
+# 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.
+
+SHELL := bash
+PYTHON ?= python3
+PRETTIER := npx prettier
+FIXTURES := "table-spec/**/*.json"
+VENV := .venv
+VENV_PYTHON := $(VENV)/bin/python
+VENV_STAMP := $(VENV)/.installed
+NODE_STAMP := node_modules/.installed
+
+.PHONY: help install format lint validate test license check clean
+
+help: ## Show available targets
+ @grep -E '^[a-zA-Z_-]+:.*## ' $(MAKEFILE_LIST) | awk 'BEGIN {FS =
":.*## "}; {printf " %-10s %s\n", $$1, $$2}'
+
+$(VENV_STAMP): dev/requirements.txt
+ $(PYTHON) -m venv $(VENV)
+ $(VENV_PYTHON) -m pip install --quiet -r dev/requirements.txt
+ @touch $(VENV_STAMP)
+
+$(NODE_STAMP): package.json package-lock.json
+ npm ci --no-audit --no-fund
+ @touch $(NODE_STAMP)
+
+install: $(VENV_STAMP) $(NODE_STAMP) ## Install the Python and Node tooling
dependencies
+
+format: $(NODE_STAMP) ## Format the JSON fixtures in place
+ $(PRETTIER) --log-level warn --write $(FIXTURES)
+
+lint: $(NODE_STAMP) ## Fail if any JSON fixture is not formatted
+ $(PRETTIER) --check $(FIXTURES)
+
+validate: $(VENV_STAMP) ## Validate every cases.json against the JSON Schemas
+ $(VENV_PYTHON) dev/validate-fixtures.py
+
+# Each malformed fixture is written into a scratch table-spec tree; the
validator
+# must reject every one.
+test: $(VENV_STAMP) ## Check the validator rejects malformed fixtures
+ @set -e; \
+ assert_reject() { \
+ d="$$(mktemp -d)"; \
+ mkdir -p "$$d/table-spec/types/x"; \
+ printf '%s' "$$2" > "$$d/table-spec/types/x/cases.json"; \
+ if $(VENV_PYTHON) dev/validate-fixtures.py "$$d" >/dev/null 2>&1;
then \
Review Comment:
`>/dev/null 2>&1` plus a bare nonzero check means a validator that crashes —
bad import, wrong path — reads as "rejected," so these asserts can all pass
without the rejection logic ever running. I'd gate on exit code 1 specifically,
or add a positive control that a known-good fixture exits 0, so we actually
know the validator ran.
##########
Makefile:
##########
@@ -0,0 +1,81 @@
+# 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.
+
+SHELL := bash
+PYTHON ?= python3
+PRETTIER := npx prettier
+FIXTURES := "table-spec/**/*.json"
Review Comment:
This matches every JSON under table-spec/, not just the fixtures — so it'll
also reformat any JSON Schema files there (which may be hand-formatted on
purpose) and still misses root JSON like the new package.json. I'd scope it to
`**/cases.json` so `make format` touches exactly the fixtures, or document the
precise set so a contributor who runs format and still trips `make lint` knows
why.
##########
.github/workflows/validate-fixtures.yml:
##########
@@ -42,29 +42,14 @@ jobs:
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 #
v6.3.0
with:
python-version: '3.12'
- - name: Install validator deps
- run: python3 -m pip install -r dev/requirements.txt
+ - name: Install Node
Review Comment:
With the pip install gone and `make validate` building its own `.venv`,
setup-python is now just putting python3 on PATH — two sources of env truth and
no dependency cache. Not blocking; either drop setup-python, or keep it and add
`cache: pip` keyed on dev/requirements.txt.
##########
CONTRIBUTING.md:
##########
@@ -268,6 +268,12 @@ to you, but it has to cover:
It has to settle the comparison rule and how a failure is labeled. Pseudocode
is one way to do that and prose is another.
+## Before opening a pull request
+
+All fixtures must be formatted by Prettier by running `make format`.
Review Comment:
Small ordering fix: `make install` should come before `make format` here,
and it's worth naming `make lint` since that's what CI fails on. Something
like: run `make install` once, then `make format`; CI runs `make lint` and will
reject unformatted fixtures.
##########
package.json:
##########
@@ -0,0 +1,7 @@
+{
+ "name": "iceberg-verification",
+ "private": true,
+ "devDependencies": {
Review Comment:
No `engines` here, so there's nothing telling a contributor which Node they
need — CI runs 22, the lockfile only says >=14. I'd add an `engines.node` floor
(`>=18` or `>=20`) to set the expectation.
##########
.github/workflows/validate-fixtures.yml:
##########
@@ -42,29 +42,14 @@ jobs:
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 #
v6.3.0
with:
python-version: '3.12'
- - name: Install validator deps
- run: python3 -m pip install -r dev/requirements.txt
+ - name: Install Node
+ uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 #
v7.0.0
+ with:
+ node-version: '22'
+ cache: npm
+ - name: Check fixture formatting
+ run: make lint
Review Comment:
Now that `make lint` is the formatting gate, I'd double-check this
workflow's `on.paths` covers the Makefile, package.json, package-lock.json and
.prettierrc.json. If it only matches `table-spec/**`/`dev/**`, a change that
breaks the lint config won't trigger CI and could merge unnoticed. The filter
isn't in the diff, so flagging to confirm.
##########
dev/.rat-excludes:
##########
@@ -19,6 +19,8 @@
LICENSE
NOTICE
lib
+.venv/**
Review Comment:
The existing entries here are bare names (LICENSE, NOTICE, lib), but these
two are globs. Depending on how check-license hands this file to RAT,
`.venv/**` may not match nested paths the way a bare `.venv` would — and since
`make check` runs license after validate has built `.venv`, a miss means RAT
scans thousands of vendored files and fails locally (CI's fine, it never builds
the venv). Worth running `make check` locally once; if it trips, use the bare
`.venv` / `node_modules` forms to match the neighbours.
##########
Makefile:
##########
@@ -0,0 +1,81 @@
+# 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.
+
+SHELL := bash
+PYTHON ?= python3
+PRETTIER := npx prettier
Review Comment:
`npx prettier` will silently fetch the latest Prettier off the network if
node_modules isn't there, defeating the 3.9.9 pin. The stamp prereq covers the
normal path, but I'd make it `npx --no-install prettier` (or
`node_modules/.bin/prettier`) so a missing install fails loudly instead of
pulling an unpinned version.
##########
Makefile:
##########
@@ -0,0 +1,81 @@
+# 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.
+
+SHELL := bash
+PYTHON ?= python3
+PRETTIER := npx prettier
+FIXTURES := "table-spec/**/*.json"
+VENV := .venv
+VENV_PYTHON := $(VENV)/bin/python
+VENV_STAMP := $(VENV)/.installed
+NODE_STAMP := node_modules/.installed
+
+.PHONY: help install format lint validate test license check clean
+
+help: ## Show available targets
+ @grep -E '^[a-zA-Z_-]+:.*## ' $(MAKEFILE_LIST) | awk 'BEGIN {FS =
":.*## "}; {printf " %-10s %s\n", $$1, $$2}'
+
+$(VENV_STAMP): dev/requirements.txt
+ $(PYTHON) -m venv $(VENV)
+ $(VENV_PYTHON) -m pip install --quiet -r dev/requirements.txt
+ @touch $(VENV_STAMP)
+
+$(NODE_STAMP): package.json package-lock.json
+ npm ci --no-audit --no-fund
+ @touch $(NODE_STAMP)
+
+install: $(VENV_STAMP) $(NODE_STAMP) ## Install the Python and Node tooling
dependencies
+
+format: $(NODE_STAMP) ## Format the JSON fixtures in place
+ $(PRETTIER) --log-level warn --write $(FIXTURES)
+
+lint: $(NODE_STAMP) ## Fail if any JSON fixture is not formatted
+ $(PRETTIER) --check $(FIXTURES)
+
+validate: $(VENV_STAMP) ## Validate every cases.json against the JSON Schemas
+ $(VENV_PYTHON) dev/validate-fixtures.py
+
+# Each malformed fixture is written into a scratch table-spec tree; the
validator
+# must reject every one.
+test: $(VENV_STAMP) ## Check the validator rejects malformed fixtures
Review Comment:
`test` reads like the main fixture validation, but this target is
specifically the validator self-test. I'd rename it to `selftest` (or
`test-validator`) and update .PHONY, `check`, and the workflow step so the name
matches what it does.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
