sungwy commented on code in PR #15: URL: https://github.com/apache/iceberg-verification/pull/15#discussion_r4190134627
########## Makefile: ########## @@ -0,0 +1,76 @@ +# 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 --yes [email protected] +FIXTURES := "table-spec/**/*.json" +VENV := .venv +VENV_PYTHON := $(VENV)/bin/python +VENV_STAMP := $(VENV)/.installed + +.PHONY: help install format lint validate test license check clean + +help: ## Show available targets Review Comment: nit: could we add a short "Before opening a PR" note to `CONTRIBUTING.md` (e.g. `make format` then `make check`)? Otherwise I think first-time fixture authors will mostly find out about the formatter from a red `lint` check. Happy for this to be a follow-up too. ########## Makefile: ########## @@ -0,0 +1,76 @@ +# 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 --yes [email protected] Review Comment: Thanks for wiring this up @rambleraptor! A couple of thoughts on how we pull in Prettier: 1. Could we add an explicit `actions/setup-node` step (SHA-pinned, with `node-version` set) to `validate-fixtures.yml`? Right now `make lint` depends on whatever Node the runner image ships, and I'd like that controlled in the workflow like our other actions. [Polaris](https://github.com/apache/polaris/blob/d9b44e1ff6dea5cc03b329ad173d7e8a3dfb461b/.github/workflows/ci.yml#L81-L86) and [DataFusion](https://github.com/apache/datafusion/blob/b40d696112e1c98c1ff5ad7c57bd8cbb001c428a/.github/workflows/rust.yml#L890-L894) both do this for their Prettier/Node checks. 2. What do you think about moving the pin into a small `package.json` + `package-lock.json` (`prettier` as a devDependency) and running `npm ci` in `make install`? Then the Makefile can just call `npx prettier`, the version lives in one place, and the install is integrity-checked against the lockfile rather than resolved fresh from npm each run. I'd consider (1) needed before merging; (2) feels like the right shape to me but happy to hear if you think it's overkill. WDYT? -- 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]
