Repository navigation
Use the _PGXNTOOL_/_ prefix for internal test-gate variables and targets - #130
Conversation
…INSTALL_ERROR_STOP_SCRIPT Internal plumbing variables are named _PGXNTOOL_*, so the name itself marks them as outside the user-facing surface while staying namespaced. This one is new in the current unreleased cycle, so no HISTORY.asc entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
check-stale-expected and check-test-install-error-stop are wired into TEST_DEPS and never meant to be invoked directly, so rename them to _check-stale-expected and _check-test-install-error-stop. The _ prefix also keeps them out of `make list`. .claude/CLAUDE.md now spells out the naming convention: PGXNTOOL_ is a user-facing override point, _PGXNTOOL_ (and _-prefixed targets) is internal-only, and the older unprefixed internal variables stay as they are because consumers may reference them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-install-error-stop-script # Conflicts: # README.html
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - Older unprefixed internal variables (`TEST_DEPS`, `TEST_SQL_FILES`, | ||
| `TEST_BUILD_*`, `REGRESS_DBNAME`, `MAJORVER`, `GE91`, `PGXNTOOL_DIR`, | ||
| `PGXNTOOL_distclean`, ...) stay as they are, since consumers may | ||
| reference them. Don't rename them; give every new internal variable the | ||
| `_PGXNTOOL_` prefix. |
There was a problem hiding this comment.
PGXNTOOL_distclean is miscategorized here. This bullet calls it an "older unprefixed internal variable," but it's neither unprefixed nor internal: it's PGXNTOOL_-prefixed, and README.asc documents it as a supported user-facing extension point (PGXNTOOL_distclean += my_generated_config.mk). That makes it API per this same section's first bullet ("PGXNTOOL_* variables are user-facing override points: document them in README.asc"), not internal plumbing.
Suggest dropping PGXNTOOL_distclean (and likely PGXNTOOL_DIR, which has the same prefix mismatch with "unprefixed") from this list, or rewording the bullet so it doesn't assert these are internal.
There was a problem hiding this comment.
🤖 (Claude) Agreed, fixed in acfee29: PGXNTOOL_distclean is removed from the internal list (it is a documented += extension point). PGXNTOOL_DIR stays, since it is genuinely internal; the bullet now says it keeps its bare prefix rather than calling it unprefixed. The paired variable inventory (pgxntool-test#97, aff5fda) now classifies PGXNTOOL_distclean as api.
Code reviewFound 1 issue:
Everything else checked out: the |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-install-error-stop-script # Conflicts: # README.html
It's internal plumbing, so it follows the _pgxntool_ convention. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
) Related pgxntool PR: Postgres-Extensions/pgxntool#130 Add `test/standard/make-variables.bats`, which lists every make variable pgxntool's `*.mk` files define (from make's own `-p` database, so no regex over base.mk) and checks it against `test/lib/pgxntool-variables.txt`.
Rename the internal pieces of the test/install and stale-expected gates so their names mark them as internal, matching the
_PGXNTOOL_convention:_CHECK_TEST_INSTALL_ERROR_STOP_SCRIPT->_PGXNTOOL_CHECK_TEST_INSTALL_ERROR_STOP_SCRIPT(new this cycle, so no HISTORY.asc entry)pgxntool_validate_choice(new this cycle) ->_pgxntool_validate_choicecheck-stale-expected->_check-stale-expectedandcheck-test-install-error-stop->_check-test-install-error-stop. Both are wired intoTEST_DEPSand never meant to be invoked directly, and the_prefix also keeps them out ofmake list.check-stale-expectedshipped in 2.3.0, so the existing STABLE_PGXNTOOL_rename entry in HISTORY.asc now mentions it..claude/CLAUDE.mdnow spells out the convention:PGXNTOOL_is a user-facing override point._PGXNTOOL_(and_-prefixed targets) is internal-only.README.html is regenerated. It was also missing the check-duplicate-docs section.
Merge order: this renames names that pgxntool-test's existing tests reference, so no single merge order is safe. Land it back-to-back with the paired pgxntool-test PR, and ideally before the next release so the misnamed variable never ships.
Related pgxntool-test PR: Postgres-Extensions/pgxntool-test#97
🤖 Generated with Claude Code